subject:"\[net\-next PATCH\] net\: bridge\: fix for bridging 802.1Q without REORDER

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

2015-09-16 Thread Phil Sutter

On Tue, Sep 15, 2015 at 10:36:41PM -0400, Vlad Yasevich wrote:
> On 09/15/2015 02:17 PM, Phil Sutter wrote:
> > 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  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 
> >>> ---
> >>>  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

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

2015-09-15 Thread Vlad Yasevich

On 09/15/2015 02:17 PM, Phil Sutter wrote:
> 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  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 
>>> ---
>>>  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

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

2015-09-15 Thread Phil Sutter

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

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

2015-09-15 Thread Vlad Yasevich

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

-vlad




> Cheers, Phil
> 

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

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

2015-09-14 Thread Vlad Yasevich

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

I wonder if it would be safe to just use skb->mac_len.

Of course, looks like vlan filtering also makes this assumption and
could be really broken.  And God forbid, someone creates a bunch of
nested encapsulated vlans (Q-in-Q-in...) with REORDER_HEADER == 0.
We could end up completely leaving the ethernet header out.

Looks like it's been there for a very long while.

-vlad

> 
> Cheers, Phil
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

2015-09-14 Thread Phil Sutter

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

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

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

2015-09-11 Thread Stephen Hemminger

On Fri, 11 Sep 2015 21:22:03 +0200
Phil Sutter  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 
> ---
>  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, this looks so special case it doesn't seem like a good idea.
Something is broken in VLAN handling if this is required.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

2015-09-11 Thread Phil Sutter

On Fri, Sep 11, 2015 at 12:24:45PM -0700, Stephen Hemminger wrote:
> On Fri, 11 Sep 2015 21:22:03 +0200
> Phil Sutter  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 
> > ---
> >  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.

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

[net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

2015-09-11 Thread Phil Sutter

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 
---
 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);
-- 
2.1.2

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

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

Re: [net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

[net-next PATCH] net: bridge: fix for bridging 802.1Q without REORDER_HDR

9 matches

Site Navigation

Mail list logo

Footer information