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.toshi...@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.doyc...@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. -- Toshiaki Makita
