[PATCH net] net: handle 802.1P vlan 0 packets properly

[PATCH net] net: handle 802.1P vlan 0 packets properly

[Govindarajulu Varadarajan]

When stack receives pkt: [802.1P vlan 0][802.1AD vlan 100][IPv4],
vlan_do_receive() returns false if it does not find vlan_dev. Later
__netif_receive_skb_core() fails to find packet type handler for
skb->protocol 801.1AD and drops the packet.

801.1P header with vlan id 0 should be handled as untagged packets.
This patch fixes it by checking if vlan_id is 0 and processes next vlan
header.

Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com>
---
 net/8021q/vlan_core.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index a313165e7a67..0cde54c02c3f 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -9,14 +9,32 @@
 bool vlan_do_receive(struct sk_buff **skbp)
 {
 	struct sk_buff *skb = *skbp;
-	__be16 vlan_proto = skb->vlan_proto;
-	u16 vlan_id = skb_vlan_tag_get_id(skb);
+	__be16 vlan_proto;
+	u16 vlan_id;
 	struct net_device *vlan_dev;
 	struct vlan_pcpu_stats *rx_stats;
 
+again:
+	vlan_proto = skb->vlan_proto;
+	vlan_id = skb_vlan_tag_get_id(skb);
 	vlan_dev = vlan_find_dev(skb->dev, vlan_proto, vlan_id);
-	if (!vlan_dev)
+	if (!vlan_dev) {
+		/* Incase of 802.1P header with vlan id 0, continue if
+		 * vlan_dev is not found.
+		 */
+		if (unlikely(!vlan_id)) {
+			__vlan_hwaccel_clear_tag(skb);
+			if (skb->protocol == cpu_to_be16(ETH_P_8021Q) ||
+			    skb->protocol == cpu_to_be16(ETH_P_8021AD)) {
+				skb = skb_vlan_untag(skb);
+				*skbp = skb;
+				if (unlikely(!skb))
+					return false;
+				goto again;
+			}
+		}
 		return false;
+	}
 
 	skb = *skbp = skb_share_check(skb, GFP_ATOMIC);
 	if (unlikely(!skb))
-- 
2.22.0

Re: [PATCH net] net: handle 802.1P vlan 0 packets properly

[David Miller]

From: Govindarajulu Varadarajan <gvaradar@cisco.com>
Date: Mon, 10 Jun 2019 07:27:02 -0700

> When stack receives pkt: [802.1P vlan 0][802.1AD vlan 100][IPv4],
> vlan_do_receive() returns false if it does not find vlan_dev. Later
> __netif_receive_skb_core() fails to find packet type handler for
> skb->protocol 801.1AD and drops the packet.
> 
> 801.1P header with vlan id 0 should be handled as untagged packets.
> This patch fixes it by checking if vlan_id is 0 and processes next vlan
> header.
> 
> Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com>

Under Linux we absolutely do not decapsulate the VLAN protocol unless
a VLAN device is configured on that interface.

Re: [PATCH net] net: handle 802.1P vlan 0 packets properly

[Stephen Suryaputra]

On Mon, Jun 10, 2019 at 02:28:10PM -0700, David Miller wrote:
> From: Govindarajulu Varadarajan <gvaradar@cisco.com>
> Date: Mon, 10 Jun 2019 07:27:02 -0700
> 
> > When stack receives pkt: [802.1P vlan 0][802.1AD vlan 100][IPv4],
> > vlan_do_receive() returns false if it does not find vlan_dev. Later
> > __netif_receive_skb_core() fails to find packet type handler for
> > skb->protocol 801.1AD and drops the packet.
> > 
> > 801.1P header with vlan id 0 should be handled as untagged packets.
> > This patch fixes it by checking if vlan_id is 0 and processes next vlan
> > header.
> > 
> > Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com>
> 
> Under Linux we absolutely do not decapsulate the VLAN protocol unless
> a VLAN device is configured on that interface.

VLAN ID 0 is treated as if the VLAN protocol isn't there. It is used so
that the 802.1 priority bits can be encoded and acted upon.

RE: [PATCH net] net: handle 802.1P vlan 0 packets properly

[Christian Benvenuti (benve)]

> -----Original Message-----
> From: Stephen Suryaputra <ssuryaextr@gmail.com>
> Sent: Monday, June 10, 2019 4:09 PM
> To: David Miller <davem@davemloft.net>
> Cc: Govindarajulu Varadarajan (gvaradar) <gvaradar@cisco.com>; Christian
> Benvenuti (benve) <benve@cisco.com>; netdev@vger.kernel.org;
> govind.varadar@gmail.com
> Subject: Re: [PATCH net] net: handle 802.1P vlan 0 packets properly
> 
> On Mon, Jun 10, 2019 at 02:28:10PM -0700, David Miller wrote:
> > From: Govindarajulu Varadarajan <gvaradar@cisco.com>
> > Date: Mon, 10 Jun 2019 07:27:02 -0700
> >
> > > When stack receives pkt: [802.1P vlan 0][802.1AD vlan 100][IPv4],
> > > vlan_do_receive() returns false if it does not find vlan_dev. Later
> > > __netif_receive_skb_core() fails to find packet type handler for
> > > skb->protocol 801.1AD and drops the packet.
> > >
> > > 801.1P header with vlan id 0 should be handled as untagged packets.
> > > This patch fixes it by checking if vlan_id is 0 and processes next
> > > vlan header.
> > >
> > > Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com>
> >
> > Under Linux we absolutely do not decapsulate the VLAN protocol unless
> > a VLAN device is configured on that interface.
> 
> VLAN ID 0 is treated as if the VLAN protocol isn't there. It is used so that the
> 802.1 priority bits can be encoded and acted upon.

David,
  if we assume that the kernel is supposed to deal properly with .1p tagged frames, regardless
of what the next header is (802.{1Q,1AD} or something else), I think the case this patch was
trying to address (that is 1Q+1AD) is not handled properly in the case of priority tagged frames
when the (1Q) vlan is untagged and therefore 1Q is only used to carry 1p.
 
    [1P vid=0][1AD].

Here is a simplified summary of how the kernel is dealing with priority frames right now, based on
- what the next protocol is
and
- whether a vlan device exists or not for the outer (1Q) header.
 
PS:
'vid' below refers to the vlan id in the 1Q header (not the 1AD header)

Case 1: vid != 0 , no nested 1Q/1AD  - OK
Case 2: vid != 0 , nested      1Q/1AD  - OK
Case 3: vid =  0 , no nested 1Q/1AD  - OK
Case 4: vid =  0 , nested       1Q/1AD <--- The patch addresses this case

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Case 1: vid != 0 , no nested 1Q/1AD
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
	Packet looks like this:

	[802.1Q  vid = V ! = 0] [Next header: Anything but 802.{1Q,1AD}]

Case 1a: Vlan device present (*)

	__netif_receive_skb_core()
	+-> skb->protocol != ETH_P_802{1Q,1AD}   <--- therefore no call to skb_vlan_untag()
	+-> vlan_do_receive()
	       +-> vlan_dev = vlan_find_dev(...) <--- vlan device found (*)
	       +-> skb->dev = vlan_dev
	+-> Deliver skb to vlan device
	
Case 1b: Vlan device NOT present (**)

	__netif_receive_skb_core()
	+-> skb->protocol != ETH_P_802{1Q,1AD}  <--- therefore no call to skb_vlan_untag()
	+-> vlan_do_receive()
	      +-> vlan_dev = vlan_find_dev(...)     <--- vlan device NOT found (**)
	+-> if (skb_vlan_tag_present(skb))         <--- TRUE
	                if (skb_vlan_tag_get_id(skb))  <--- TRUE (***)
		          PACKET_OTHERHOST
		  __vlan_hwaccel_clear_tag(skb)
	+-> Deliver pkt to next layer protocol
		If we take the example of IPv4, ip_rcv_core() will drop the pkt because of
	 	PACKET_OTHERHOST.

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Case 2: vid != 0 , nested 1Q/1AD
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
	Packet looks like this:

	[802.1Q  vid = V ! = 0] [Next header: 802.{1Q,1AD}]

Case 2a: Vlan device present

	+-> if (skb->protocol == ETH_P_8021AD) <--- TRUE
	        +-> skb_vlan_untag(skb)
		+-> if unlikely(skb_vlan_tag_present(skb)) <--- TRUE
		               return skb
	        +-> if (skb_vlan_tag_present(skb)) <--- TRUE (again)
			if (vlan_do_receive(&skb)) <--- TRUE
			     +-> vlan_dev = vlan_find_dev(...) <--- vlan device found
			     +-> skb->dev = vlan_dev

				goto another_round <-- At the following round the packet will be delivered to the next layer proto

Case 2b: Vlan device NOT present

	__netif_receive_skb_core()
	+-> if (skb->protocol == ETH_P_8021AD)  <--- TRUE
	       +-> skb_vlan_untag(skb)
		+-> If (unlikely(skb_vlan_tag_present(skb)) <--- TRUE (packet is .1p tagged, vid=0)
		               return skb <--- packet is returned as is, 1AD header still inline
	        +-> if (skb_vlan_tag_present(skb)) <--- TRUE (again)
			if (vlan_do_receive(&skb)) <--- FALSE
			     +-> vlan_dev = vlan_find_dev(...) <--- vlan device NOT found

	        The 2nd part is the same as 1b:

	+-> if (skb_vlan_tag_present(skb))         <--- TRUE
	                if (skb_vlan_tag_get_id(skb))  <--- TRUE (***)
		          PACKET_OTHERHOST
		  __vlan_hwaccel_clear_tag(skb)
	+-> Deliver pkt to next layer protocol
		If we take the example of IPv4, ip_rcv_core() will drop the pkt because of
		PACKET_OTHERHOST.

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Case 3: vid = 0 , no nested 1Q/1AD
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
	Packet looks like this:

	[802.1Q  vid = 0][Next header: Anything but 802.{1Q,1AD}]

Case 3a: vlan (0) device present

	Same as case 1a: packet delivered to vlan device.

Case 3b: vlan (0) device not present

	This is similar to case 1b above, with the difference that condition (***) is false
	and therefore pkt type is NOT set to PACKET_OTHERHOST, which translates to
	".1p / vid==0 header ignored and packet NOT accepted".

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Case 4: vid = 0 , nested 1Q/1AD
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
	Packet looks like this:

	[802.1Q  vid == 0][Next header: 802.{1Q,1AD}]

Case 4a: vlan (0) device present

	This is equivalent to the cases 1a/2a/3a above: the packet is going to be delivered to the vlan (0) device.

Case 4b: vlan (0) device not present.
	THIS IS THE CASE THE PATCH TRIED TO ADDRESS.

	__netif_receive_skb_core()
	+-> if (skb->protocol == ETH_P_8021AD)  <--- TRUE
	       +-> skb_vlan_untag(skb)
		+-> If (unlikely(skb_vlan_tag_present(skb)) <--- TRUE (packet is .1p tagged, vid=0)
		               return skb <--- packet is returned as is, 1AD header still inline

 	+-> Since there is no 1AD proto handler, packet will be dropped (but it should not)

Thanks
/Chris

Re: [PATCH net] net: handle 802.1P vlan 0 packets properly

[David Miller]

From: "Christian Benvenuti (benve)" <benve@cisco.com>
Date: Tue, 11 Jun 2019 00:35:59 +0000

>   if we assume that the kernel is supposed to deal properly with .1p tagged frames, regardless
> of what the next header is (802.{1Q,1AD} or something else), I think the case this patch was
> trying to address (that is 1Q+1AD) is not handled properly in the case of priority tagged frames
> when the (1Q) vlan is untagged and therefore 1Q is only used to carry 1p.
>  
>     [1P vid=0][1AD].
> 
> Here is a simplified summary of how the kernel is dealing with priority frames right now, based on
> - what the next protocol is
> and
> - whether a vlan device exists or not for the outer (1Q) header.

Yeah I think I misunderstood the situation.

Please repost the patch.

Thanks.

Re: [PATCH net] net: handle 802.1P vlan 0 packets properly

[Govindarajulu Varadarajan (gvaradar)]

On Mon, 2019-06-10 at 14:28 -0700, David Miller wrote:
> From: Govindarajulu Varadarajan <gvaradar@cisco.com>
> Date: Mon, 10 Jun 2019 07:27:02 -0700
> 
> > When stack receives pkt: [802.1P vlan 0][802.1AD vlan 100][IPv4],
> > vlan_do_receive() returns false if it does not find vlan_dev. Later
> > __netif_receive_skb_core() fails to find packet type handler for
> > skb->protocol 801.1AD and drops the packet.
> > 
> > 801.1P header with vlan id 0 should be handled as untagged packets.
> > This patch fixes it by checking if vlan_id is 0 and processes next vlan
> > header.
> > 
> > Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com>
> 
> Under Linux we absolutely do not decapsulate the VLAN protocol unless
> a VLAN device is configured on that interface.

Can you clarify on if 802.1P header (with vlan id 0) not supported with inner
802.1Q or 802.1AD header?

We already decapsulate vlan 0 header (802.1P) for all other inner protocols if
vlan 0 device is not present.

For example: pkt - [802.1P vlan 0][IPv4]

In __netif_receive_skb_core():

	if (vlan_do_receive(&skb))
		goto another_round;

vlan_do_receive() returns false since it did not find VLAN 0 device.

Later, we decapsulate vlan 0 header (802.1P) here:

        if (unlikely(skb_vlan_tag_present(skb))) {
                if (skb_vlan_tag_get_id(skb))	//False, because vlan id is 0
                        skb->pkt_type = PACKET_OTHERHOST;
                /* Note: we might in the future use prio bits
                 * and set skb->priority like in vlan_do_receive()
                 * For the time being, just ignore Priority Code Point
                 */
                __vlan_hwaccel_clear_tag(skb);
        }

Then we deliver packet to packet type handler (IPv4) here:

        /* deliver only exact match when indicated */
        if (likely(!deliver_exact)) {
                deliver_ptype_list_skb(skb, &pt_prev, orig_dev, type,
                                       &ptype_base[ntohs(type) &
                                                   PTYPE_HASH_MASK]);
        }

        deliver_ptype_list_skb(skb, &pt_prev, orig_dev, type,
                               &orig_dev->ptype_specific);

This does not work if skb->protocol (inner) is 802.1Q or 802.1AD. Since they do
not have packet type handler, pkt is dropped.

Default configuration of our hardware is to priority tag all packets. IMO 802.1P
header with (vlan id 0) should be treated as priority tagged packets, and not as
a vlan tagged packet.

Alternative is, enic driver not setting skb->vlan_tci if vlan_id is 0. IMO driver
should forward pkt as it sees on wire. And stack should handle the headers
properly.

Is there any other way to solve the problem?