Re: [PATCH] vlan: Fix duplicate delivery of vlan 0 packets to ETH_P_ALL packet sockets

[Jesse Gross <jesse@nicira.com>]

On Mon, Mar 21, 2011 at 2:35 PM, Eric W. Biederman
<ebiederm@xmission.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?

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.