| Message ID | 20190610142702.2698-1-gvaradar@cisco.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: handle 802.1P vlan 0 packets properly | expand |
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.
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?
> -----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
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.
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))
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(-)