netdev - Re: [PATCH] vlan: Fix duplicate delivery of vlan 0 packets to ETH_P

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 23 Mar 2011 13:59:25 -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 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?

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.
--
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