| Message ID | 20190113135939.8970-2-zahari.doychev@linux.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: bridge: fix tc added QinQ forwarding | expand |
On 2019/01/13 22:59, Zahari Doychev wrote: > Use the skb->mac_len instead of using the ETH_HLEN when pushing the skb > data pointer. This fixes sending incorrect packets when more than one > vlan tags are pushed by tc-vlan and the mac header length is bigger than > ETH_HLEN. In this way the vlan tagged contained in the skb is inserted at > right offset in the packet payload before sending the packet. > > Signed-off-by: Zahari Doychev <zahari.doychev@linux.com> > --- > net/bridge/br_forward.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c > index 5372e2042adf..55f928043f77 100644 > --- a/net/bridge/br_forward.c > +++ b/net/bridge/br_forward.c > @@ -39,7 +39,7 @@ int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb > if (!is_skb_forwardable(skb->dev, skb)) > goto drop; > > - skb_push(skb, ETH_HLEN); > + skb_push(skb, skb->mac_len); > br_drop_fake_rtable(skb); > > if (skb->ip_summed == CHECKSUM_PARTIAL && > I guess you mean skb->data points to mac_header + ETH_HLEN + VLAN_HLEN when bridge receives skbs in br_handle_frame()? If so, the behavior of act_vlan is odd. Normal double tagged skbs from hardware devices should have skb->data pointing to mac_header + ETH_HLEN because they just call eth_type_trans() before entering netif_receive_skb()... I think act_vlan needs some fix.
On Tue, Jan 15, 2019 at 03:11:28PM +0900, Toshiaki Makita wrote: > On 2019/01/13 22:59, Zahari Doychev wrote: > > Use the skb->mac_len instead of using the ETH_HLEN when pushing the skb > > data pointer. This fixes sending incorrect packets when more than one > > vlan tags are pushed by tc-vlan and the mac header length is bigger than > > ETH_HLEN. In this way the vlan tagged contained in the skb is inserted at > > right offset in the packet payload before sending the packet. > > > > Signed-off-by: Zahari Doychev <zahari.doychev@linux.com> > > --- > > net/bridge/br_forward.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c > > index 5372e2042adf..55f928043f77 100644 > > --- a/net/bridge/br_forward.c > > +++ b/net/bridge/br_forward.c > > @@ -39,7 +39,7 @@ int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb > > if (!is_skb_forwardable(skb->dev, skb)) > > goto drop; > > > > - skb_push(skb, ETH_HLEN); > > + skb_push(skb, skb->mac_len); > > br_drop_fake_rtable(skb); > > > > if (skb->ip_summed == CHECKSUM_PARTIAL && > > > > I guess you mean skb->data points to mac_header + ETH_HLEN + VLAN_HLEN > when bridge receives skbs in br_handle_frame()? yes, this is what I see. > If so, the behavior of act_vlan is odd. Normal double tagged skbs from > hardware devices should have skb->data pointing to mac_header + ETH_HLEN > because they just call eth_type_trans() before entering > netif_receive_skb()... > I think act_vlan needs some fix. The act_valn is using the skb_vlan_push(...) to add the vlan tags and in this way increasing the skb->data and mac_len. So I think I can add a fix there to set the skb->data to point to mac_header + ETH_HLEN when more tags are added. Thanks Zahari > > -- > Toshiaki Makita >
On 2019/01/17 17:17, Zahari Doychev wrote: > On Tue, Jan 15, 2019 at 03:11:28PM +0900, Toshiaki Makita wrote: >> On 2019/01/13 22:59, Zahari Doychev wrote: >>> Use the skb->mac_len instead of using the ETH_HLEN when pushing the skb >>> data pointer. This fixes sending incorrect packets when more than one >>> vlan tags are pushed by tc-vlan and the mac header length is bigger than >>> ETH_HLEN. In this way the vlan tagged contained in the skb is inserted at >>> right offset in the packet payload before sending the packet. >>> >>> Signed-off-by: Zahari Doychev <zahari.doychev@linux.com> >>> --- >>> net/bridge/br_forward.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c >>> index 5372e2042adf..55f928043f77 100644 >>> --- a/net/bridge/br_forward.c >>> +++ b/net/bridge/br_forward.c >>> @@ -39,7 +39,7 @@ int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb >>> if (!is_skb_forwardable(skb->dev, skb)) >>> goto drop; >>> >>> - skb_push(skb, ETH_HLEN); >>> + skb_push(skb, skb->mac_len); >>> br_drop_fake_rtable(skb); >>> >>> if (skb->ip_summed == CHECKSUM_PARTIAL && >>> >> >> I guess you mean skb->data points to mac_header + ETH_HLEN + VLAN_HLEN >> when bridge receives skbs in br_handle_frame()? > > yes, this is what I see. > >> If so, the behavior of act_vlan is odd. Normal double tagged skbs from >> hardware devices should have skb->data pointing to mac_header + ETH_HLEN >> because they just call eth_type_trans() before entering >> netif_receive_skb()... >> I think act_vlan needs some fix. > > The act_valn is using the skb_vlan_push(...) to add the vlan tags and in this > way increasing the skb->data and mac_len. So I think I can add a fix there to > set the skb->data to point to mac_header + ETH_HLEN when more tags are added. As skb->data always points to mac_header after calling skb_vlan_push(), we probably need to remember mac_len before invocation of it? The problem should be this part in tcf_vlan_act(): > out: > if (skb_at_tc_ingress(skb)) > skb_pull_rcsum(skb, skb->mac_len); skb->mac_len should not be used here.
On Thu, Jan 17, 2019 at 12:59 AM Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote: > > On 2019/01/17 17:17, Zahari Doychev wrote: > > On Tue, Jan 15, 2019 at 03:11:28PM +0900, Toshiaki Makita wrote: > >> On 2019/01/13 22:59, Zahari Doychev wrote: > >>> Use the skb->mac_len instead of using the ETH_HLEN when pushing the skb > >>> data pointer. This fixes sending incorrect packets when more than one > >>> vlan tags are pushed by tc-vlan and the mac header length is bigger than > >>> ETH_HLEN. In this way the vlan tagged contained in the skb is inserted at > >>> right offset in the packet payload before sending the packet. > >>> > >>> Signed-off-by: Zahari Doychev <zahari.doychev@linux.com> > >>> --- > >>> net/bridge/br_forward.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c > >>> index 5372e2042adf..55f928043f77 100644 > >>> --- a/net/bridge/br_forward.c > >>> +++ b/net/bridge/br_forward.c > >>> @@ -39,7 +39,7 @@ int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb > >>> if (!is_skb_forwardable(skb->dev, skb)) > >>> goto drop; > >>> > >>> - skb_push(skb, ETH_HLEN); > >>> + skb_push(skb, skb->mac_len); > >>> br_drop_fake_rtable(skb); > >>> > >>> if (skb->ip_summed == CHECKSUM_PARTIAL && > >>> > >> > >> I guess you mean skb->data points to mac_header + ETH_HLEN + VLAN_HLEN > >> when bridge receives skbs in br_handle_frame()? > > > > yes, this is what I see. > > > >> If so, the behavior of act_vlan is odd. Normal double tagged skbs from > >> hardware devices should have skb->data pointing to mac_header + ETH_HLEN > >> because they just call eth_type_trans() before entering > >> netif_receive_skb()... > >> I think act_vlan needs some fix. > > > > The act_valn is using the skb_vlan_push(...) to add the vlan tags and in this > > way increasing the skb->data and mac_len. So I think I can add a fix there to > > set the skb->data to point to mac_header + ETH_HLEN when more tags are added. > > As skb->data always points to mac_header after calling skb_vlan_push(), > we probably need to remember mac_len before invocation of it? > > The problem should be this part in tcf_vlan_act(): > > > out: > > if (skb_at_tc_ingress(skb)) > > skb_pull_rcsum(skb, skb->mac_len); > > skb->mac_len should not be used here. I am confused. This code is to push skb->data back to network header. If skb_vlan_push() pushes skb->data to mac header, then this code is correct for pulling it back to network header, as skb->mac_len is updated accordingly inside skb_vlan_push() too. What goes wrong here? skb->mac_len isn't correct for double tagging? Thanks.
On 2019/01/18 4:19, Cong Wang wrote: > On Thu, Jan 17, 2019 at 12:59 AM Toshiaki Makita > <makita.toshiaki@lab.ntt.co.jp> wrote: >> On 2019/01/17 17:17, Zahari Doychev wrote: >>> On Tue, Jan 15, 2019 at 03:11:28PM +0900, Toshiaki Makita wrote: >>>> On 2019/01/13 22:59, Zahari Doychev wrote: >>>>> Use the skb->mac_len instead of using the ETH_HLEN when pushing the skb >>>>> data pointer. This fixes sending incorrect packets when more than one >>>>> vlan tags are pushed by tc-vlan and the mac header length is bigger than >>>>> ETH_HLEN. In this way the vlan tagged contained in the skb is inserted at >>>>> right offset in the packet payload before sending the packet. >>>>> >>>>> Signed-off-by: Zahari Doychev <zahari.doychev@linux.com> >>>>> --- >>>>> net/bridge/br_forward.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c >>>>> index 5372e2042adf..55f928043f77 100644 >>>>> --- a/net/bridge/br_forward.c >>>>> +++ b/net/bridge/br_forward.c >>>>> @@ -39,7 +39,7 @@ int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb >>>>> if (!is_skb_forwardable(skb->dev, skb)) >>>>> goto drop; >>>>> >>>>> - skb_push(skb, ETH_HLEN); >>>>> + skb_push(skb, skb->mac_len); >>>>> br_drop_fake_rtable(skb); >>>>> >>>>> if (skb->ip_summed == CHECKSUM_PARTIAL && >>>>> >>>> >>>> I guess you mean skb->data points to mac_header + ETH_HLEN + VLAN_HLEN >>>> when bridge receives skbs in br_handle_frame()? >>> >>> yes, this is what I see. >>> >>>> If so, the behavior of act_vlan is odd. Normal double tagged skbs from >>>> hardware devices should have skb->data pointing to mac_header + ETH_HLEN >>>> because they just call eth_type_trans() before entering >>>> netif_receive_skb()... >>>> I think act_vlan needs some fix. >>> >>> The act_valn is using the skb_vlan_push(...) to add the vlan tags and in this >>> way increasing the skb->data and mac_len. So I think I can add a fix there to >>> set the skb->data to point to mac_header + ETH_HLEN when more tags are added. >> >> As skb->data always points to mac_header after calling skb_vlan_push(), >> we probably need to remember mac_len before invocation of it? >> >> The problem should be this part in tcf_vlan_act(): >> >>> out: >>> if (skb_at_tc_ingress(skb)) >>> skb_pull_rcsum(skb, skb->mac_len); >> >> skb->mac_len should not be used here. > > I am confused. This code is to push skb->data back to network header. > If skb_vlan_push() pushes skb->data to mac header, then this code > is correct for pulling it back to network header, as skb->mac_len is > updated accordingly inside skb_vlan_push() too. > > What goes wrong here? skb->mac_len isn't correct for double tagging? Bridge and VLAN code (skb_vlan_untag in __netif_receive_skb_core) expects skb->data to point to the start of VLAN header, not the next (network) header. After calling tcf_vlan_act() on ingress filter, skb->data points to the next of VLAN header (network header), while non-hwaccel VLAN packets (including double tagged ones) from NIC drivers have skb->data pointing to the start of VLAN header as expected. I'm not sure if mac_len should or should not be updated in skb_vlan_push(). Anyway IIRC skb_vlan_untag() and bridge code do not rely on mac_len so mac_len should not cause problems there.
On Fri, Jan 18, 2019 at 11:29:52AM +0900, Toshiaki Makita wrote: > On 2019/01/18 4:19, Cong Wang wrote: > > On Thu, Jan 17, 2019 at 12:59 AM Toshiaki Makita > > <makita.toshiaki@lab.ntt.co.jp> wrote: > >> On 2019/01/17 17:17, Zahari Doychev wrote: > >>> On Tue, Jan 15, 2019 at 03:11:28PM +0900, Toshiaki Makita wrote: > >>>> On 2019/01/13 22:59, Zahari Doychev wrote: > >>>>> Use the skb->mac_len instead of using the ETH_HLEN when pushing the skb > >>>>> data pointer. This fixes sending incorrect packets when more than one > >>>>> vlan tags are pushed by tc-vlan and the mac header length is bigger than > >>>>> ETH_HLEN. In this way the vlan tagged contained in the skb is inserted at > >>>>> right offset in the packet payload before sending the packet. > >>>>> > >>>>> Signed-off-by: Zahari Doychev <zahari.doychev@linux.com> > >>>>> --- > >>>>> net/bridge/br_forward.c | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c > >>>>> index 5372e2042adf..55f928043f77 100644 > >>>>> --- a/net/bridge/br_forward.c > >>>>> +++ b/net/bridge/br_forward.c > >>>>> @@ -39,7 +39,7 @@ int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb > >>>>> if (!is_skb_forwardable(skb->dev, skb)) > >>>>> goto drop; > >>>>> > >>>>> - skb_push(skb, ETH_HLEN); > >>>>> + skb_push(skb, skb->mac_len); > >>>>> br_drop_fake_rtable(skb); > >>>>> > >>>>> if (skb->ip_summed == CHECKSUM_PARTIAL && > >>>>> > >>>> > >>>> I guess you mean skb->data points to mac_header + ETH_HLEN + VLAN_HLEN > >>>> when bridge receives skbs in br_handle_frame()? > >>> > >>> yes, this is what I see. > >>> > >>>> If so, the behavior of act_vlan is odd. Normal double tagged skbs from > >>>> hardware devices should have skb->data pointing to mac_header + ETH_HLEN > >>>> because they just call eth_type_trans() before entering > >>>> netif_receive_skb()... > >>>> I think act_vlan needs some fix. > >>> > >>> The act_valn is using the skb_vlan_push(...) to add the vlan tags and in this > >>> way increasing the skb->data and mac_len. So I think I can add a fix there to > >>> set the skb->data to point to mac_header + ETH_HLEN when more tags are added. > >> > >> As skb->data always points to mac_header after calling skb_vlan_push(), > >> we probably need to remember mac_len before invocation of it? > >> > >> The problem should be this part in tcf_vlan_act(): > >> > >>> out: > >>> if (skb_at_tc_ingress(skb)) > >>> skb_pull_rcsum(skb, skb->mac_len); > >> > >> skb->mac_len should not be used here. > > > > I am confused. This code is to push skb->data back to network header. > > If skb_vlan_push() pushes skb->data to mac header, then this code > > is correct for pulling it back to network header, as skb->mac_len is > > updated accordingly inside skb_vlan_push() too. > > > > What goes wrong here? skb->mac_len isn't correct for double tagging? > > Bridge and VLAN code (skb_vlan_untag in __netif_receive_skb_core) > expects skb->data to point to the start of VLAN header, not the next > (network) header. After calling tcf_vlan_act() on ingress filter, > skb->data points to the next of VLAN header (network header), while > non-hwaccel VLAN packets (including double tagged ones) from NIC drivers > have skb->data pointing to the start of VLAN header as expected. > > I'm not sure if mac_len should or should not be updated in > skb_vlan_push(). Anyway IIRC skb_vlan_untag() and bridge code do not > rely on mac_len so mac_len should not cause problems there. Replacing the usage of the skb->maclen in act vlan with ETH_HLEN in the skb_push/pull calls fixes the problem. I have to do some more testing then I can send a new patch. I still see the problem in br_vlan __allowed_ingress when the skb->vlan_proto and br->vlan_proto does not match. If the network header is not reset then the flow dissector does not correctly detect the vlan tags. The the network header points to the second tag where as the skb->data points to the first one and the egress rule in my example cannot match. Is this the correct way to fix this and is __allowed_ingress the correct place? Thanks Zahari > > -- > Toshiaki Makita >
On 2019/01/22 6:11, Zahari Doychev wrote: > On Fri, Jan 18, 2019 at 11:29:52AM +0900, Toshiaki Makita wrote: >> On 2019/01/18 4:19, Cong Wang wrote: >>> On Thu, Jan 17, 2019 at 12:59 AM Toshiaki Makita >>> <makita.toshiaki@lab.ntt.co.jp> wrote: >>>> On 2019/01/17 17:17, Zahari Doychev wrote: >>>>> On Tue, Jan 15, 2019 at 03:11:28PM +0900, Toshiaki Makita wrote: >>>>>> On 2019/01/13 22:59, Zahari Doychev wrote: >>>>>>> Use the skb->mac_len instead of using the ETH_HLEN when pushing the skb >>>>>>> data pointer. This fixes sending incorrect packets when more than one >>>>>>> vlan tags are pushed by tc-vlan and the mac header length is bigger than >>>>>>> ETH_HLEN. In this way the vlan tagged contained in the skb is inserted at >>>>>>> right offset in the packet payload before sending the packet. >>>>>>> >>>>>>> Signed-off-by: Zahari Doychev <zahari.doychev@linux.com> >>>>>>> --- >>>>>>> net/bridge/br_forward.c | 2 +- >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c >>>>>>> index 5372e2042adf..55f928043f77 100644 >>>>>>> --- a/net/bridge/br_forward.c >>>>>>> +++ b/net/bridge/br_forward.c >>>>>>> @@ -39,7 +39,7 @@ int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb >>>>>>> if (!is_skb_forwardable(skb->dev, skb)) >>>>>>> goto drop; >>>>>>> >>>>>>> - skb_push(skb, ETH_HLEN); >>>>>>> + skb_push(skb, skb->mac_len); >>>>>>> br_drop_fake_rtable(skb); >>>>>>> >>>>>>> if (skb->ip_summed == CHECKSUM_PARTIAL && >>>>>>> >>>>>> >>>>>> I guess you mean skb->data points to mac_header + ETH_HLEN + VLAN_HLEN >>>>>> when bridge receives skbs in br_handle_frame()? >>>>> >>>>> yes, this is what I see. >>>>> >>>>>> If so, the behavior of act_vlan is odd. Normal double tagged skbs from >>>>>> hardware devices should have skb->data pointing to mac_header + ETH_HLEN >>>>>> because they just call eth_type_trans() before entering >>>>>> netif_receive_skb()... >>>>>> I think act_vlan needs some fix. >>>>> >>>>> The act_valn is using the skb_vlan_push(...) to add the vlan tags and in this >>>>> way increasing the skb->data and mac_len. So I think I can add a fix there to >>>>> set the skb->data to point to mac_header + ETH_HLEN when more tags are added. >>>> >>>> As skb->data always points to mac_header after calling skb_vlan_push(), >>>> we probably need to remember mac_len before invocation of it? >>>> >>>> The problem should be this part in tcf_vlan_act(): >>>> >>>>> out: >>>>> if (skb_at_tc_ingress(skb)) >>>>> skb_pull_rcsum(skb, skb->mac_len); >>>> >>>> skb->mac_len should not be used here. >>> >>> I am confused. This code is to push skb->data back to network header. >>> If skb_vlan_push() pushes skb->data to mac header, then this code >>> is correct for pulling it back to network header, as skb->mac_len is >>> updated accordingly inside skb_vlan_push() too. >>> >>> What goes wrong here? skb->mac_len isn't correct for double tagging? >> >> Bridge and VLAN code (skb_vlan_untag in __netif_receive_skb_core) >> expects skb->data to point to the start of VLAN header, not the next >> (network) header. After calling tcf_vlan_act() on ingress filter, >> skb->data points to the next of VLAN header (network header), while >> non-hwaccel VLAN packets (including double tagged ones) from NIC drivers >> have skb->data pointing to the start of VLAN header as expected. >> >> I'm not sure if mac_len should or should not be updated in >> skb_vlan_push(). Anyway IIRC skb_vlan_untag() and bridge code do not >> rely on mac_len so mac_len should not cause problems there. > > Replacing the usage of the skb->maclen in act vlan with ETH_HLEN in the > skb_push/pull calls fixes the problem. I have to do some more testing then > I can send a new patch. Using the original mac_len (before entering skb_vlan_push()) would be safer, since mac_len is not necessarily be ETH_HLEN (with reorder_hdr off, setting vlan_protocol in tun_pi, ...). Actually a number of VLAN-related code assumes ETH_HLEN, though... > I still see the problem in br_vlan __allowed_ingress when the skb->vlan_proto > and br->vlan_proto does not match. If the network header is not reset then the > flow dissector does not correctly detect the vlan tags. The the network header > points to the second tag where as the skb->data points to the first one and the > egress rule in my example cannot match. Is this the correct way to fix this > and is __allowed_ingress the correct place? network_header needs to point to L3 (IP) header, at least with CHECKSUM_PARTIAL. network_header offset is fixed up in br_dev_queue_push_xmit() for such packets, so the fix at __allowed_ingress() would not be a complete fix. Requiring network_header to be at VLAN header is not compatible with drivers that support CHECKSUM_PARTIAL. I guess something needs to be fixed in flower, if it has such a requirement.
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c index 5372e2042adf..55f928043f77 100644 --- a/net/bridge/br_forward.c +++ b/net/bridge/br_forward.c @@ -39,7 +39,7 @@ int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb if (!is_skb_forwardable(skb->dev, skb)) goto drop; - skb_push(skb, ETH_HLEN); + skb_push(skb, skb->mac_len); br_drop_fake_rtable(skb); if (skb->ip_summed == CHECKSUM_PARTIAL &&
Use the skb->mac_len instead of using the ETH_HLEN when pushing the skb data pointer. This fixes sending incorrect packets when more than one vlan tags are pushed by tc-vlan and the mac header length is bigger than ETH_HLEN. In this way the vlan tagged contained in the skb is inserted at right offset in the packet payload before sending the packet. Signed-off-by: Zahari Doychev <zahari.doychev@linux.com> --- net/bridge/br_forward.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)