[PATCH 0/2] net: bridge: fix tc added QinQ forwarding

[PATCH 0/2] net: bridge: fix tc added QinQ forwarding

[Zahari Doychev]

The Linux bridge seems to not correctly forward double vlan tagged packets
added using the tc vlan action. I am using a bridge with two netdevs and on one
of them a have the clsact qdisc with tc flower rule adding two vlan
tags.

ip link add name br0 type bridge vlan_filtering 1
ip link set dev br0 up

ip link set dev net0 up
ip link set dev net0 master br0

ip link set dev net1 up
ip link set dev net1 master br0

bridge vlan add dev net0 vid 100 master
bridge vlan add dev br0 vid 100 self
bridge vlan add dev net1 vid 100 master

tc qdisc add dev net0 handle ffff: clsact
tc qdisc add dev net1 handle ffff: clsact

tc filter add dev net0 ingress pref 1 protocol all flower \
		  action vlan push id 10 pipe action vlan push id 100

tc filter add dev net0 egress pref 1 protocol 802.1q flower \
		  vlan_id 100 vlan_ethtype 802.1q cvlan_id 10 \
		  action vlan pop pipe action vlan pop

When using the setup above the packets coming on net0 get double tagged but
the MAC headers gets corrupted when the packets go out of net1. It seems that
the second vlan header is not considered in br_dev_queue_push_xmit. The skb
data pointer is decremented only by the ethernet header length. This later
causes the function validate_xmit_vlan to insert the outer vlan tag behind
the inner vlan tag. The inner vlan becomes also part of the source mac address.

The first patch fixes the problem described above. The second one fixes
similar problem when the tpids of the bridge and the inserted vlan don't match.
It fixes again incorrect insertion of the skb vlan into the payload. The two
patches seem to fix the problem but I am not sure if this the right way to fix
this and if there is any other impact.

Zahari Doychev (2):
  net: bridge: fix tc added QinQ forwarding
  net: bridge: fix tc added vlan insert as payload

 net/bridge/br_forward.c | 2 +-
 net/bridge/br_vlan.c    | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.20.1

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

[Zahari Doychev]

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 &&
-- 
2.20.1

[PATCH 2/2] net: bridge: fix tc added vlan insert as payload

[Zahari Doychev]

The skb->mac_len is used for the skb_push before inserting the vlan tag to
the packet payload. After that the skb is pulled only ETH_HLEN and the
network header is being reset to get the correct packet mac header. This
avoids sending packets with incorrect mac header when vlan tags are pushed
with tc-vlan and the bridge tpid does not match the inserted vlan tpid. The
vlan tag is inserted at the right place of the header.

Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
---
 net/bridge/br_vlan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 4a2f31157ef5..f857487245c6 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -460,13 +460,14 @@ static bool __allowed_ingress(const struct net_bridge *br,
 		/* Tagged frame */
 		if (skb->vlan_proto != br->vlan_proto) {
 			/* Protocol-mismatch, empty out vlan_tci for new tag */
-			skb_push(skb, ETH_HLEN);
+			skb_push(skb, skb->mac_len);
 			skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
 							skb_vlan_tag_get(skb));
 			if (unlikely(!skb))
 				return false;
 
 			skb_pull(skb, ETH_HLEN);
+			skb_reset_network_header(skb);
 			skb_reset_mac_len(skb);
 			*vid = 0;
 			tagged = false;
-- 
2.20.1

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

[Nikolay Aleksandrov]

On 13/01/2019 15:59, Zahari Doychev wrote:
> The Linux bridge seems to not correctly forward double vlan tagged packets
> added using the tc vlan action. I am using a bridge with two netdevs and on one
> of them a have the clsact qdisc with tc flower rule adding two vlan
> tags.
> 
> ip link add name br0 type bridge vlan_filtering 1
> ip link set dev br0 up
> 
> ip link set dev net0 up
> ip link set dev net0 master br0
> 
> ip link set dev net1 up
> ip link set dev net1 master br0
> 
> bridge vlan add dev net0 vid 100 master
> bridge vlan add dev br0 vid 100 self
> bridge vlan add dev net1 vid 100 master
> 
> tc qdisc add dev net0 handle ffff: clsact
> tc qdisc add dev net1 handle ffff: clsact
> 
> tc filter add dev net0 ingress pref 1 protocol all flower \
> 		  action vlan push id 10 pipe action vlan push id 100
> 
> tc filter add dev net0 egress pref 1 protocol 802.1q flower \
> 		  vlan_id 100 vlan_ethtype 802.1q cvlan_id 10 \
> 		  action vlan pop pipe action vlan pop
> 
> When using the setup above the packets coming on net0 get double tagged but
> the MAC headers gets corrupted when the packets go out of net1. It seems that
> the second vlan header is not considered in br_dev_queue_push_xmit. The skb
> data pointer is decremented only by the ethernet header length. This later
> causes the function validate_xmit_vlan to insert the outer vlan tag behind
> the inner vlan tag. The inner vlan becomes also part of the source mac address.
> 
> The first patch fixes the problem described above. The second one fixes
> similar problem when the tpids of the bridge and the inserted vlan don't match.
> It fixes again incorrect insertion of the skb vlan into the payload. The two
> patches seem to fix the problem but I am not sure if this the right way to fix
> this and if there is any other impact.
> 
> Zahari Doychev (2):
>   net: bridge: fix tc added QinQ forwarding
>   net: bridge: fix tc added vlan insert as payload
> 
>  net/bridge/br_forward.c | 2 +-
>  net/bridge/br_vlan.c    | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 

How well was this set tested ? It breaks connectivity between bridge and
members when vlans are used. The host generated packets going out of the bridge
have mac_len = 0.

E.g.:
# tcpdump -e -n -i vnet2
tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
listening on vnet2, link-type EN10MB (Ethernet), capture size 262144 bytes
17:47:08.824208 00:01:52:54:00:04 > 00:01:08:00:06:04, ethertype 802.1Q (0x8100), length 32: vlan 25, p 0, ethertype 0x5eba, 
	0x0000:  c0a8 6401 0000 0000 0000 c0a8 6402       ..d.........d.
17:47:09.848492 00:01:52:54:00:04 > 00:01:08:00:06:04, ethertype 802.1Q (0x8100), length 32: vlan 25, p 0, ethertype 0x5eba, 
	0x0000:  c0a8 6401 0000 0000 0000 c0a8 6402       ..d.........d.

Headers are messed up. This is br0 with pvid 25 and vnet2 with vlan 25 tagged.
You'll probably have to reset mac_len in bridge's xmit function.

The potentially affected cases will need to be carefully considered, as you've noted,
since this change is dangerous. Please also run all bridge selftests and make sure
they all pass. Giving more details about which cases you've considered and how
this set was tested in the commit log would be very helpful.

Thanks,
 Nik

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

[Zahari Doychev]

On Mon, Jan 14, 2019 at 01:46:09PM +0200, Nikolay Aleksandrov wrote:
> On 13/01/2019 15:59, Zahari Doychev wrote:

[...]

> 
> How well was this set tested ? It breaks connectivity between bridge and
> members when vlans are used. The host generated packets going out of the bridge
> have mac_len = 0.
> 
> E.g.:
> # tcpdump -e -n -i vnet2
> tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
> listening on vnet2, link-type EN10MB (Ethernet), capture size 262144 bytes
> 17:47:08.824208 00:01:52:54:00:04 > 00:01:08:00:06:04, ethertype 802.1Q (0x8100), length 32: vlan 25, p 0, ethertype 0x5eba, 
> 	0x0000:  c0a8 6401 0000 0000 0000 c0a8 6402       ..d.........d.
> 17:47:09.848492 00:01:52:54:00:04 > 00:01:08:00:06:04, ethertype 802.1Q (0x8100), length 32: vlan 25, p 0, ethertype 0x5eba, 
> 	0x0000:  c0a8 6401 0000 0000 0000 c0a8 6402       ..d.........d.
> 
> Headers are messed up. This is br0 with pvid 25 and vnet2 with vlan 25 tagged.
> You'll probably have to reset mac_len in bridge's xmit function.
> 

Thanks I will look into this. I have done most of tests with PC with two nics
and two other hosts conntected to each of the interfaces.

> The potentially affected cases will need to be carefully considered, as you've noted,
> since this change is dangerous. Please also run all bridge selftests and make sure
> they all pass. Giving more details about which cases you've considered and how
> this set was tested in the commit log would be very helpful.
> 

Thanks for pointing to the selftests. I will run them. These are the tests part
of the kselftest correct?

Regards,
Zahari

> Thanks,
>  Nik
> 
> 

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

[Toshiaki Makita]

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.

-- 
Toshiaki Makita

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

[Zahari Doychev]

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
> 

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

[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.

-- 
Toshiaki Makita

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

[Cong Wang]

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.

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

[Toshiaki Makita]

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.

-- 
Toshiaki Makita

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

[Zahari Doychev]

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
> 

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

[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.

-- 
Toshiaki Makita