Re: [PATCH] net: do not pass vlan pkts to real dev pkt handler also

["Nicolas de Pesloüan" <nicolas.2p.debian@gmail.com>]

Le 13/12/2011 22:45, Jiri Pirko a écrit :
> Tue, Dec 13, 2011 at 06:11:03PM CET, vasu.dev@linux.intel.com wrote:
>> On Tue, 2011-12-13 at 15:21 +0100, Jiri Pirko wrote:
>>> Tue, Dec 13, 2011 at 02:08:52AM CET, vasu.dev@linux.intel.com wrote:
>>>> On Mon, 2011-12-12 at 23:56 +0100, Jiri Pirko wrote:
>>>>> Mon, Dec 12, 2011 at 11:19:23PM CET, vasu.dev@intel.com wrote:
>>>>>> The orig_dev has to be updated before going another round
>>>>>> for vlan pkts, otherwise currently unmodified real orig_dev
>>>>>> causes vlan pkt delivered to real orig_dev also.
>>>>>>
>>>>>> The fcoe stack doesn't expects its vlan pkts on real dev
>>>>>> and it causes crash in fcoe stack.
>>>>>
>>>>> Could you please provide more info on where exactly it would crash and
>>>>> why?
>>>>
>>>> Its in fcoe stack due to its fip rx skb list getting corrupt as same skb
>>>> instance getting queued twice without being cloned, though list was well
>>>> protected by its spin lock, it was queued on its two fcoe instances, one
>>>> on real dev and other on its vlan.
>>>>
>>>> I could also handle this gracefully in fcoe stack by cloning but any
>>>> case netdev should not forward vlan pkt to its read dev pkt handler also
>>>> and that is getting fixed with this patch, so patch will restore
>>>> orig_dev uses for *only* vlan pkts as it was with recursive
>>>> __netif_receive_skb calling prior to commit 0dfe178.
>>>
>>>
>>> I do not see into fcoe code, but wouldn't it be good to do skb
>>> skb_share_check in fcoe_ctlr_recv? I suppose that would solve your
>>> problem and looks legal to me.
>>>
>>
>> Yes that will fix along with dropping vlan pkts on real dev, so some
>> additional checking for dropping also. In fact that is what I meant in
>> my last response by "I could also handle this gracefully in fcoe stack
>> by cloning" as skb_share_check() does that conditionally.
>>
>> But as far as this patch goes, are you okay with the fix to not forward
>> vlan pkt on real dev pkt handler ? I think this is required regardless
>> of fcoe stack fixing for shared skb since otherwise all upper layers of
>> real dev pkt handler has to handle with un-expected vlan pkts also.
>
> I think that's what orig_dev is destined for. To provide a possiblility
> to do this. I would like to leave that as it is.

I agree with Jiri.

If a protocol handler is registered on a particular device (instead of NULL), then the handler will 
receive whatever is received on this device. This is true for bridge, for bonding and probably for 
all other "stackable" devices. I don't see any reason to handle it in a different way for vlan.

	Nicolas.