Re: [Bridge] [PATCH 1/2] net: bridge: fix tc added QinQ forwarding

[Zahari Doychev <zahari.doychev@linux.com>]

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
>