Date:	Tue, 15 Sep 2015 20:17:55 +0200
From:	Phil Sutter <phil@....cc>
To:	Vlad Yasevich <vyasevich@...il.com>
Cc:	Stephen Hemminger <stephen@...workplumber.org>,
	bridge@...ts.linux-foundation.org, netdev@...r.kernel.org
Subject: Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without
 REORDER_HDR

On Tue, Sep 15, 2015 at 11:11:53AM -0400, Vlad Yasevich wrote:
> On 09/14/2015 04:06 PM, Phil Sutter wrote:
> > On Mon, Sep 14, 2015 at 02:21:10PM -0400, Vlad Yasevich wrote:
> >> On 09/11/2015 04:20 PM, Phil Sutter wrote:
> >>> On Fri, Sep 11, 2015 at 12:24:45PM -0700, Stephen Hemminger wrote:
> >>>> On Fri, 11 Sep 2015 21:22:03 +0200
> >>>> Phil Sutter <phil@....cc> wrote:
> >>>>
> >>>>> When forwarding packets from an 802.1Q interface with REORDER_HDR set to
> >>>>> zero, the VLAN header previously inserted by vlan_do_receive() needs to
> >>>>> be stripped from the packet and the mac_header adjustment undone,
> >>>>> otherwise a tagged frame with first four bytes missing will be
> >>>>> transmitted.
> >>>>>
> >>>>> Signed-off-by: Phil Sutter <phil@....cc>
> >>>>> ---
> >>>>>  net/bridge/br_input.c | 10 ++++++++++
> >>>>>  1 file changed, 10 insertions(+)
> >>>>>
> >>>>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> >>>>> index f921a5d..e4e3fc7 100644
> >>>>> --- a/net/bridge/br_input.c
> >>>>> +++ b/net/bridge/br_input.c
> >>>>> @@ -288,6 +288,16 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> >>>>>  	}
> >>>>>  
> >>>>>  forward:
> >>>>> +	if (is_vlan_dev(skb->dev) &&
> >>>>> +	    !(vlan_dev_priv(skb->dev)->flags & VLAN_FLAG_REORDER_HDR)) {
> >>>>> +		unsigned int offset = skb->data - skb_mac_header(skb);
> >>>>> +
> >>>>> +		skb_push(skb, offset);
> >>>>> +		memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
> >>>>> +		skb->mac_header += VLAN_HLEN;
> >>>>> +		skb_pull(skb, offset);
> >>>>> +		skb_reset_mac_len(skb);
> >>>>> +	}
> >>>>>  	switch (p->state) {
> >>>>>  	case BR_STATE_FORWARDING:
> >>>>>  		rhook = rcu_dereference(br_should_route_hook);
> >>>>
> >>>> Thanks for finding this. Is this a new thing or has it always been there?
> >>>
> >>> Sorry, I didn't check if this is a regression or not. Seen initially
> >>> with RHEL7's kernel-3.10.0-229.7.2, which due to the massive backporting
> >>> is by far not as old as it might seem. But it's surely not a brand new
> >>> problem of net-next or so.
> >>>
> >>> Since nowadays no sane mind touches REORDER_HDR (there was originally a
> >>> bug in NetworkManager which defaulted this to 0), it may very well be
> >>> there for a long time already.
> >>>
> >>>> Sorry, this looks so special case it doesn't seem like a good idea.
> >>>> Something is broken in VLAN handling if this is required.
> >>>
> >>> It is so ugly, I wish I had found a better way to fix the problem. Well,
> >>> maybe I miss something:
> >>>
> >>> - packet enters __netif_receive_skb_core():
> >>>   - skb->protocol is set to ETH_P_8021Q, so:
> >>>     - packet is untagged
> >>>     - skb->vlan_tci set
> >>>     - skb->protocol set to 'real' protocol
> >>>   - skb_vlan_tag_present(skb) == true, so:
> >>>     - vlan_do_receive() is called:
> >>>       - tags the packet again
> >>>       - zeroes vlan_tci
> >>>     - goto another_round
> >>> - __netif_receive_skb_core(), round 2:
> >>>   - skb->protocol is not ETH_P_8021Q -> no untagging
> >>>   - skb_vlan_tag_present(skb) == false -> no vlan_do_receive()
> >>>   - rx_handler handler (== br_handle_frame) is called
> >>>
> >>> IMO the root of all evil is the existence of REORDER_HDR itself. It
> >>> causes an skb which should have been untagged to being passed along with
> >>> VLAN header present and code dealing with it needs to clean up the mess.
> >>
> >> So the problem here appears the be the code the in br_dev_queue_push_xmit().
> >> It assumes that MAC_HLEN worth of data has been removed from the skb,
> >> which is normal in case of normal VLAN processing.  However, without
> >> REORDER_HEADER set this is no longer the case.  In this case, the ethernet
> >> header is shifted 4 bytes, and when we push the it back we miss the 4 bytes
> >> of the destination mac address...
> > 
> > Please note that vlan_do_receive() also inserts the VLAN header in
> > between ethernet header and IP header, therefore:
> > 
> >> I wonder if it would be safe to just use skb->mac_len.
> > 
> > Given this works, the bridge would still forward a tagged frame which
> > should have been untagged in the first place.
> > 
> > I just wondered where this added VLAN header is dropped if the interface
> > does not belong to a bridge, but then realized that further packet
> > processing simply ignores the ethernet header (and everything following
> > it). So unless I forget something, this should indeed be a
> > bridge-specific problem.
> > 
> 
> Looks like macvtap is also susceptible to this problem.  It seems to be a bad
> idea to allow any upper device configuration on top of a REORDER_HDR=0 vlan.
> It is also not enough to just check is_vlan_dev(skb->dev) because vlan may be at
> lower in the device stack.

Oh well. Apart from implementing workarounds for this workaround, maybe
there is another approach:

If I see this correctly, the VLAN header is only interesting for users
looking directly at the packet, like PACKET or RAW sockets. All others
either ignore the MAC layer or expect it to be untagged (which is the
purpose of reading packets from a VLAN interface).

Users wanting to read these (re-)tagged frames need to attach to the
interface where they occur directly. Expecting to see the VLAN header
when sniffing on a bridge interface which has the problematic VLAN
interface enslaved can sanely be denied, right?

So wouldn't it be the easier option to patch up these users interested
in the data to insert the tag there instead of having it added by
default and deciding where (and how) to get rid of it again?

Assuming that e.g. a PACKET socket is bound to the interface, everything
necessary should be at hand, i.e. if REORDER_HDR is set or not and what
the VLAN ID was.

What do you think?

Cheers, Phil
--
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