Message-ID: <AANLkTinFdUdxTF1qWr22YGsqw+bXeMYiV7QHwpXioxd3@mail.gmail.com>
Date:	Fri, 1 Apr 2011 18:41:34 -0700
From:	Jesse Gross <jesse@...ira.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	Michał Mirosław <mirqus@...il.com>,
	Ben Hutchings <bhutchings@...arflare.com>,
	Eric Dumazet <eric.dumazet@...il.com>,
	John Fastabend <john.r.fastabend@...el.com>
Subject: Re: [PATCH] vlan: Fix duplicate delivery of vlan 0 packets to
 ETH_P_ALL packet sockets

On Sat, Mar 26, 2011 at 11:27 PM, Eric W. Biederman
<ebiederm@...ssion.com> wrote:
> Jesse Gross <jesse@...ira.com> writes:
>
>> On Mon, Mar 21, 2011 at 2:35 PM, Eric W. Biederman
>> <ebiederm@...ssion.com> wrote:
>>>
>>> For vlan data coming in from nics without vlan hardware accelleration we
>>> get two copies of vlan packets with vlan id 0 on pf_packet sockets, causing
>>> userspace to break.  This is caused by delivering the same packet to the same
>>> networking device more than once.
>>
>> I agree that this is a problem and the code consolidation is very nice
>> but I'm concerned that there is extra complexity for the rest of the
>> system to counterbalance what is saved here.
>>
>>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>>> index ce8e3ab..a0849b9 100644
>>> --- a/net/8021q/vlan_core.c
>>> +++ b/net/8021q/vlan_core.c
>>> +void emulate_vlan_hwaccel(struct sk_buff *skb)
>>> +{
>>> +       struct vlan_hdr *vhdr = (struct vlan_hdr *)skb->data;
>>> +       __be16 proto;
>>> +
>>> +       if (!pskb_may_pull(skb, VLAN_HLEN))
>>> +               return;
>>> +
>>> +       __vlan_hwaccel_put_tag(skb, vhdr->h_vlan_TCI);
>>> +       skb_pull_rcsum(skb, VLAN_HLEN);
>>
>> Doesn't this break things which push the header back on?  Bridging
>> pushes ETH_HLEN before forwarding but here it will be a garbage value
>> due to the extra vlan header.  AF_PACKET pushes the mac header back
>> on, which in this case includes the original vlan header.  However,
>> since we've also put the tag in skb->vlan_tci, won't it appear to be
>> double tagged?
>
> Probably that part does indeed look like a bug, and my testing certainly
> shows that there are problems with my patch.
>
>> More generally, even though we pull the tag off the skb it's pretty
>> common on the receive path to look backwards into previous headers.
>> Given that this can happen, I think it's somewhat confusing/fragile to
>> have packet data which effectively should not be there.  It also adds
>> a third case to any generic vlan handling code: tag in packet (can
>> still happen, such as on transmit), received on vlan accelerated NIC -
>> tag in skb but not in packet, receive on non-vlan accelerated NIC -
>> tag in both skb and packet.
>>
>> If we actually removed the tag in the emulated case that would avoid
>> these concerns but would, of course, add extra overhead in some
>> situations.
>
> The only extra overhead I can really see is the need to put the vlan
> tag back on in a few instances.   Moving the ethernet addresses around
> in the packet (the cost of adding/removing the vlan header) since they
> are in a hot cacheline doesn't concern me very much.

Yes, you're probably right.  It likely rare that we hit this path at
all, unlikely that we'll need to put it back, and, as you say, it's in
a hot cacheline.  In that case, it seems like a reasonable approach to
me.

>
> But we definitely need to do something to fix the regression for
> pf_packet sockets.

I agree.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists