Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Changli Gao <xiaosuo@gmail.com>]

On Wed, Mar 2, 2011 at 4:11 AM, Nicolas de Pesloüan
<nicolas.2p.debian@gmail.com> wrote:
> Le 01/03/2011 16:12, Changli Gao a écrit :
>>
>> On Tue, Mar 1, 2011 at 5:29 PM, Jiri Pirko<jpirko@redhat.com>  wrote:
>>>
>>> Unapplicable, sorry (wrong branch :(). Here's corrected patch:
>>>
>>> Subject: [PATCH net-next-2.6 v2] bonding: remove skb_share_check in
>>> handle_frame
>>>
>>> No need to do share check here.
>>>
>>
>> I don't think so. Although you avoid netif_rx(), you can't avoid
>> ptype_all handlers. In fact, all the RX handlers should has this
>> check(), if they may modify the skb.
>
> Can you please develop your explanation?
>
> In current __netif_receive_skb() (after the recent patch from Jiri), we
> deliver the skb to ptype_all handlers inside a loop, while possibly changing
> skb->dev inside this loop.
>
> Then, at the end of __netif_receive_skb(), we loop on ptype_base, without
> changing anything in skb.
>
> Should we consider ptype_*->func() to be called in a pure sequential way?
> Should we consider that when a ptype_*->func() returns, nothing from this
> handler will use the skb in anyway later, in a parallel way?
>
> Or should we, instead, consider that special precautions must be taken,
> because protocol handlers may run in parallel for the same skb? Which kind
> of precautions?
>

If the packets gotten by __netif_receive_skb() are unshared, the skb
gotten by bond should be unshared, as we call prev_pt before calling
bond. I don't see there is any  relationship with the previous patch
from Jiri. The bridge is in the same condition with bond here, and it
checks if the skb is shared or not. Does it imply that dev->rx_handler
may see shared skbs?

BTW: bond may change skb and packet data. But before change the packet
data, it doesn't check if the packet data is shared or not.

1519         if (bond_dev->priv_flags & IFF_MASTER_ALB &&
1520             bond_dev->priv_flags & IFF_BRIDGE_PORT &&
1521             skb->pkt_type == PACKET_HOST) {
1522                 u16 *dest = (u16 *) eth_hdr(skb)->h_dest;

skb_cow_head() should be added, otherwise the previous ptype handler
may get the unexpected dset MAC address.

1523
1524                 memcpy(dest, bond_dev->dev_addr, ETH_ALEN);
1525         }

Thanks.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)