| Message ID | 20190610183122.4521-1-gvaradar@cisco.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RESEND,net] net: handle 802.1P vlan 0 packets properly | expand |
On 2019/06/11 3:31, Govindarajulu Varadarajan wrote: > 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); Looks like this changes existing behavior. Priority-tagged packets will be untagged before bridge, etc. I think priority-tagged packets should be forwarded as priority-tagged (iff bridge is not vlan-aware), not untagged. > + 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)) > Toshiaki Makita
@On Tue, 2019-06-11 at 13:34 +0900, Toshiaki Makita wrote: > On 2019/06/11 3:31, Govindarajulu Varadarajan wrote: > > 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); > > Looks like this changes existing behavior. Priority-tagged packets will be > untagged > before bridge, etc. I think priority-tagged packets should be forwarded as > priority-tagged > (iff bridge is not vlan-aware), not untagged. Makes sense to me. If rx_handler is registered to the device, pkt should be sent untagged to rx_handler. I would like to get some clarification on few more cases before I change the code. In the setup: br0 | vlan 100 | |(802.1AD) | | | +--------------------+ | eth0 | +--------------------+ Case 1: [802.1P vlan0] [IP] Current behaviour: pkt is sent to br0 with priority tag. i.e we should not remove the 802.1P tag. This patch: removes the 802.1P tag and br0 receives untagged packet. This is wrong. Expected behaviour: Should be same as current behaviour. Case 2: [802.1AD vlan 100] [IP] Current behaviour: pkt is sent to vlan 100 device. This patch: same as current behaviour. Expected behaviour: same as current behaviour Case 3: [802.1P vlan 0] [802.1AD vlan 100] [IP] Current behaviour: Pkt is sent to br0 rx_handler. This happens because vlan_do_receive() returns false (vlan 0 device is not present). Stack does not go through inner headers. This patch: pkt is sent to vlan 100 device. Because vlan_do_receive() strips vlan 0 header and finds vlan 100 device. Expected behaviour: ? IMO: Pkt should be sent to vlan 100 device because 802.1P should be treated as priority tag and not as vlan tagged pkt. Since vlan 100 device is present, it should be sent to vlan 100 device. Case 4: [802.1AD vlan 200] [802.1AD vlan 100] [IP] Current behaviour: Pkt is sent to br0 since vlan 200 device is not found. This patch: same as current behaviour. Expected behaviour: Same as current behaviour. Is my understanding correct?
On 2019/06/12 6:57, Govindarajulu Varadarajan (gvaradar) wrote: > @On Tue, 2019-06-11 at 13:34 +0900, Toshiaki Makita wrote: >> On 2019/06/11 3:31, Govindarajulu Varadarajan wrote: >>> 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); >> >> Looks like this changes existing behavior. Priority-tagged packets will be >> untagged >> before bridge, etc. I think priority-tagged packets should be forwarded as >> priority-tagged >> (iff bridge is not vlan-aware), not untagged. > > Makes sense to me. If rx_handler is registered to the device, pkt should be sent > untagged to rx_handler. > > > I would like to get some clarification on few more cases before I change the > code. In the setup: > > br0 > | > vlan 100 | > |(802.1AD) | > | | > +--------------------+ > | eth0 | > +--------------------+ > > Case 1: [802.1P vlan0] [IP] > Current behaviour: pkt is sent to br0 with priority tag. i.e we should not remove > the 802.1P tag. > This patch: removes the 802.1P tag and br0 receives untagged packet. This is > wrong. > Expected behaviour: Should be same as current behaviour. > > Case 2: [802.1AD vlan 100] [IP] > Current behaviour: pkt is sent to vlan 100 device. > This patch: same as current behaviour. > Expected behaviour: same as current behaviour > > Case 3: [802.1P vlan 0] [802.1AD vlan 100] [IP] > Current behaviour: Pkt is sent to br0 rx_handler. This happens because > vlan_do_receive() returns false (vlan 0 device is not present). Stack does not go > through inner headers. > This patch: pkt is sent to vlan 100 device. Because vlan_do_receive() strips vlan > 0 header and finds vlan 100 device. > Expected behaviour: ? > IMO: Pkt should be sent to vlan 100 device because 802.1P should be treated as > priority tag and not as vlan tagged pkt. Since vlan 100 device is present, it > should be sent to vlan 100 device. Maybe yes, maybe no. There is no standard about that. Your opinion is consistent behavior between untagged and priority-tagged. OTOH, it changes existing behavior. We basically try to keep existing behavior even if the behavior looks odd in some way in order not to break existing users. So I would choose the other option, send packets to br0. > > Case 4: [802.1AD vlan 200] [802.1AD vlan 100] [IP] > Current behaviour: Pkt is sent to br0 since vlan 200 device is not found. > This patch: same as current behaviour. > Expected behaviour: Same as current behaviour. > > Is my understanding correct? Agree except case 3. Toshiaki Makita
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(-)