[PATCH 0/2] Fix issues with vlans without REORDER_HEADER

[PATCH 0/2] Fix issues with vlans without REORDER_HEADER

[Vladislav Yasevich]

A while ago Phil Sutter brought up an issue with vlans without
REORDER_HEADER and bridges.  The problem was that if a vlan
without REORDER_HEADER was a port in the bridge, the bridge ended
up forwarding corrupted packets that still contained the vlan header.
The same issue exists for bridge mode macvlan/macvtap devices.

An additional issue with vlans without REORDER_HEADER is that stacking
them also doesn't work.  The reason here is that skb_reorder_vlan_header()
function assumes that it on ETH_HLEN bytes deep into the packet.  That
is not the case, when you a vlan without REORRDER_HEADER flag set.

This series attempts to correct these 2 issues.

1) To solve the stacked vlans problem, the patch simply use
skb->mac_len as an offset to start copying mac addresses that
is part of header reordering.

2) To fix the issue with bridge/macvlan/macvtap, the second patch
simply doesn't write the vlan header back to the packet if the
vlan device is either a bridge or a macvlan port.  This ends up
being the simplest and least performance intrussive solution.

I've considered extending patch 2 to all stacked devices (essentially
checked for the presense of rx_handler), but that feels like a broader
restriction and _may_ break existing uses.  

Thanks
-vlad

Vladislav Yasevich (2):
  vlan: Fix untag operations of stacked vlans with REORDER_HEADER off
  vlan: Do not put vlan headers back on bridge and macvlan ports

 include/linux/netdevice.h | 5 +++++
 net/8021q/vlan_core.c     | 4 +++-
 net/core/skbuff.c         | 3 ++-
 3 files changed, 10 insertions(+), 2 deletions(-)

-- 
1.9.3

[PATCH 1/2] vlan: Fix untag operations of stacked vlans with REORDER_HEADER off

[Vladislav Yasevich]

When we have multiple stacked vlan devices all of which have
turned off REORDER_HEADER flag, the untag operation does not
locate the ethernet addresses correctly for nested vlans.
The reason is that in case of REORDER_HEADER flag being off,
the outer vlan headers are put back and the mac_len is adjusted
to account for the presense of the header.  Then, the subsequent
untag operation, for the next level vlan, always use VLAN_ETH_HLEN
to locate the begining of the ethernet header and that ends up
being a multiple of 4 bytes short of the actuall beginning
of the mac header (the multiple depending on the how many vlan
encapsulations ethere are).

As a reslult, if there are multiple levles of vlan devices
with REODER_HEADER being off, the recevied packets end up
being dropped.

To solve this, we use skb->mac_len as the offset.  The value
is always set on receive path and starts out as a ETH_HLEN.
The value is also updated when the vlan header manupations occur
so we know it will be correct.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 net/core/skbuff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index fab4599..160193f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4268,7 +4268,8 @@ static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
 		return NULL;
 	}
 
-	memmove(skb->data - ETH_HLEN, skb->data - VLAN_ETH_HLEN, 2 * ETH_ALEN);
+	memmove(skb->data - ETH_HLEN, skb->data - skb->mac_len,
+		2 * ETH_ALEN);
 	skb->mac_header += VLAN_HLEN;
 	return skb;
 }
-- 
1.9.3

Re: [PATCH 1/2] vlan: Fix untag operations of stacked vlans with REORDER_HEADER off

[Nicolas Dichtel]

Le 16/11/2015 21:43, Vladislav Yasevich a écrit :
> When we have multiple stacked vlan devices all of which have
> turned off REORDER_HEADER flag, the untag operation does not
> locate the ethernet addresses correctly for nested vlans.
> The reason is that in case of REORDER_HEADER flag being off,
> the outer vlan headers are put back and the mac_len is adjusted
> to account for the presense of the header.  Then, the subsequent
> untag operation, for the next level vlan, always use VLAN_ETH_HLEN
> to locate the begining of the ethernet header and that ends up
> being a multiple of 4 bytes short of the actuall beginning
> of the mac header (the multiple depending on the how many vlan
> encapsulations ethere are).
>
> As a reslult, if there are multiple levles of vlan devices
> with REODER_HEADER being off, the recevied packets end up
> being dropped.
>
> To solve this, we use skb->mac_len as the offset.  The value
> is always set on receive path and starts out as a ETH_HLEN.
> The value is also updated when the vlan header manupations occur
> so we know it will be correct.
>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>   net/core/skbuff.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index fab4599..160193f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4268,7 +4268,8 @@ static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
>   		return NULL;
>   	}
>
> -	memmove(skb->data - ETH_HLEN, skb->data - VLAN_ETH_HLEN, 2 * ETH_ALEN);
> +	memmove(skb->data - ETH_HLEN, skb->data - skb->mac_len,
> +		2 * ETH_ALEN);
>   	skb->mac_header += VLAN_HLEN;
>   	return skb;
>   }
>
This patch breaks the following test case: a vlan packet is received by an
e1000 interface. Here is the configuration of the interface:
$ ethtool -k ntfp2 | grep "vlan\|offload"
tcp-segmentation-offload: off
udp-fragmentation-offload: off [fixed]
generic-segmentation-offload: on
generic-receive-offload: on
large-receive-offload: off [fixed]
rx-vlan-offload: off
tx-vlan-offload: off [fixed]
rx-vlan-filter: on [fixed]
vlan-challenged: off [fixed]
tx-vlan-stag-hw-insert: off [fixed]
rx-vlan-stag-hw-parse: off [fixed]
rx-vlan-stag-filter: off [fixed]
l2-fwd-offload: off [fixed]

The vlan header is not removed by the driver. It calls dev_gro_receive() which
sets the network header to +14, thus mac_len is also sets to 14 and
skb_reorder_vlan_header() do a wrong memmove() (the packet is dropped).
Not sure who is responsible to update mac_len before skb_vlan_untag() is
called. Any suggestions?

[PATCH net] skbuff: Fix offset error in skb_reorder_vlan_header

[Vladislav Yasevich]

skb_reorder_vlan_header is called after the vlan header has
been pulled.  As a result the offset of the begining of
the mac header has been incrased by 4 bytes (VLAN_HLEN).
When moving the mac addresses, include this incrase in
the offset calcualation so that the mac addresses are
copied correctly.

Fixes: a6e18ff1117 (vlan: Fix untag operations of stacked vlans with REORDER_HEADER off)
CC: Nicolas Dichtel <nicolas.dichtel@6wind.com>
CC: Patrick McHardy <kaber@trash.net>
Signed-off-by: Vladislav Yasevich <vyasevich@gmail.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 152b9c7..5cc43d37 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4268,7 +4268,7 @@ static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
 		return NULL;
 	}
 
-	memmove(skb->data - ETH_HLEN, skb->data - skb->mac_len,
+	memmove(skb->data - ETH_HLEN, skb->data - skb->mac_len - VLAN_HLEN,
 		2 * ETH_ALEN);
 	skb->mac_header += VLAN_HLEN;
 	return skb;
-- 
2.1.0

Re: [PATCH net] skbuff: Fix offset error in skb_reorder_vlan_header

[David Miller]

From: Vladislav Yasevich <vyasevich@gmail.com>
Date: Mon, 14 Dec 2015 17:44:10 -0500

> skb_reorder_vlan_header is called after the vlan header has
> been pulled.  As a result the offset of the begining of
> the mac header has been incrased by 4 bytes (VLAN_HLEN).
> When moving the mac addresses, include this incrase in
> the offset calcualation so that the mac addresses are
> copied correctly.
> 
> Fixes: a6e18ff1117 (vlan: Fix untag operations of stacked vlans with REORDER_HEADER off)
> CC: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> CC: Patrick McHardy <kaber@trash.net>
> Signed-off-by: Vladislav Yasevich <vyasevich@gmail.com>

Applied and queued up for -stable, thanks Vlad.

Re: [PATCH net] skbuff: Fix offset error in skb_reorder_vlan_header

[Nicolas Dichtel]

Le 14/12/2015 23:44, Vladislav Yasevich a écrit :
> skb_reorder_vlan_header is called after the vlan header has
> been pulled.  As a result the offset of the begining of
> the mac header has been incrased by 4 bytes (VLAN_HLEN).
> When moving the mac addresses, include this incrase in
> the offset calcualation so that the mac addresses are
> copied correctly.
>
> Fixes: a6e18ff1117 (vlan: Fix untag operations of stacked vlans with REORDER_HEADER off)
> CC: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> CC: Patrick McHardy <kaber@trash.net>
> Signed-off-by: Vladislav Yasevich <vyasevich@gmail.com>
Thank you for the fix. A bit late, but for the record:

Tested-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

[PATCH 2/2] vlan: Do not put vlan headers back on bridge and macvlan ports

[Vladislav Yasevich]

When a vlan is configured with REORDER_HEADER set to 0, the vlan
header is put back into the packet and makes it appear that
the vlan header is still there even after it's been processed.
This posses a problem for bridge and macvlan ports.  The packets
passed to those device may be forwarded and at the time of the
forward, vlan headers end up being unexpectedly present.

With the patch, we make sure that we do not put the vlan header
back (when REORDER_HEADER is 0) if a bridge or macvlan has
been configured on top of the vlan device.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 include/linux/netdevice.h | 5 +++++
 net/8021q/vlan_core.c     | 4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2d15e38..2f9d47e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3842,6 +3842,11 @@ static inline bool netif_is_bridge_master(const struct net_device *dev)
 	return dev->priv_flags & IFF_EBRIDGE;
 }
 
+static inline bool netif_is_bridge_port(const struct net_device *dev)
+{
+	return dev->priv_flags & IFF_BRIDGE_PORT;
+}
+
 static inline bool netif_is_ovs_master(const struct net_device *dev)
 {
 	return dev->priv_flags & IFF_OPENVSWITCH;
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 61bf2a0..c2c9a0f 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -30,7 +30,9 @@ bool vlan_do_receive(struct sk_buff **skbp)
 			skb->pkt_type = PACKET_HOST;
 	}
 
-	if (!(vlan_dev_priv(vlan_dev)->flags & VLAN_FLAG_REORDER_HDR)) {
+	if (!(vlan_dev_priv(vlan_dev)->flags & VLAN_FLAG_REORDER_HDR) &&
+	    !netif_is_macvlan_port(vlan_dev) &&
+	    !netif_is_bridge_port(vlan_dev)) {
 		unsigned int offset = skb->data - skb_mac_header(skb);
 
 		/*
-- 
1.9.3

Re: [PATCH 0/2] Fix issues with vlans without REORDER_HEADER

[David Miller]

From: Vladislav Yasevich <vyasevich@gmail.com>
Date: Mon, 16 Nov 2015 15:43:43 -0500

> A while ago Phil Sutter brought up an issue with vlans without
> REORDER_HEADER and bridges.  The problem was that if a vlan
> without REORDER_HEADER was a port in the bridge, the bridge ended
> up forwarding corrupted packets that still contained the vlan header.
> The same issue exists for bridge mode macvlan/macvtap devices.
> 
> An additional issue with vlans without REORDER_HEADER is that stacking
> them also doesn't work.  The reason here is that skb_reorder_vlan_header()
> function assumes that it on ETH_HLEN bytes deep into the packet.  That
> is not the case, when you a vlan without REORRDER_HEADER flag set.
> 
> This series attempts to correct these 2 issues.
> 
> 1) To solve the stacked vlans problem, the patch simply use
> skb->mac_len as an offset to start copying mac addresses that
> is part of header reordering.
> 
> 2) To fix the issue with bridge/macvlan/macvtap, the second patch
> simply doesn't write the vlan header back to the packet if the
> vlan device is either a bridge or a macvlan port.  This ends up
> being the simplest and least performance intrussive solution.
> 
> I've considered extending patch 2 to all stacked devices (essentially
> checked for the presense of rx_handler), but that feels like a broader
> restriction and _may_ break existing uses.  

Series applied, thanks for working on this Vlad.