[net-next,v2] net: dsa: Support bridge 802.1Q while untagging

The intent of 412a1526d067 ("net: dsa: untag the bridge pvid from rx
skbs") is to transparently untag the bridge's default_pvid when the
Ethernet switch can only support egress tagged of that default_pvid
towards the CPU port.

Prior to this commit, users would have to configure an 802.1Q upper on
the bridge master device when the bridge is configured with
vlan_filtering=0 in order to pop the VLAN tag:

ip link add name br0 type bridge vlan_filtering 0
ip link add link br0 name br0.1 type vlan id 1

After this commit we added support for managing a switch port 802.1Q
upper but those are not usually added as bridge members, and if they do,
they do not actually require any special management, the data path would
pop the desired VLAN tag accordingly.

What we want to preserve is that use case and to manage when the user
creates that 802.1Q upper for the bridge port.

While we are it, call __vlan_find_dev_deep_rcu() which makes use the
VLAN group array which is faster.

As soon as we return the VLAN tagged SKB though it will be used by the
following call path:

netif_receive_skb_list_internal
  -> __netif_receive_skb_list_core
    -> __netif_receive_skb_core
      -> vlan_do_receive()

which uses skb->vlan_proto, if we do not set it to the appropriate VLAN
protocol, we will leave it set to what the DSA master has set
(ETH_P_XDSA).

Fixes: 412a1526d067 ("net: dsa: untag the bridge pvid from rx skbs")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:

- removed unused list_head iter argument

 net/dsa/dsa_priv.h | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

On Wed, Sep 30, 2020 at 08:06:23PM -0700, Florian Fainelli wrote:
> The intent of 412a1526d067 ("net: dsa: untag the bridge pvid from rx
> skbs") is to transparently untag the bridge's default_pvid when the
> Ethernet switch can only support egress tagged of that default_pvid
> towards the CPU port.
> 
> Prior to this commit, users would have to configure an 802.1Q upper on
> the bridge master device when the bridge is configured with
> vlan_filtering=0 in order to pop the VLAN tag:
> 
> ip link add name br0 type bridge vlan_filtering 0
> ip link add link br0 name br0.1 type vlan id 1
> 
> After this commit we added support for managing a switch port 802.1Q
> upper but those are not usually added as bridge members, and if they do,
> they do not actually require any special management, the data path would
> pop the desired VLAN tag accordingly.
> 
> What we want to preserve is that use case and to manage when the user
> creates that 802.1Q upper for the bridge port.
> 
> While we are it, call __vlan_find_dev_deep_rcu() which makes use the
> VLAN group array which is faster.
> 
> As soon as we return the VLAN tagged SKB though it will be used by the
> following call path:
> 
> netif_receive_skb_list_internal
>   -> __netif_receive_skb_list_core
>     -> __netif_receive_skb_core
>       -> vlan_do_receive()
> 
> which uses skb->vlan_proto, if we do not set it to the appropriate VLAN
> protocol, we will leave it set to what the DSA master has set
> (ETH_P_XDSA).
> 

The explanation is super confusing, although I think the placement of
the "skb->vlan_proto = vlan_dev_vlan_proto(upper_dev)" is correct.
Here's what I think is going on. It has to do with what's upwards of the
code you're changing:

	/* Move VLAN tag from data to hwaccel */
	if (!skb_vlan_tag_present(skb) && hdr->h_vlan_proto == htons(proto)) {
		skb = skb_vlan_untag(skb);
		if (!skb)
			return NULL;
	}

So skb->vlan_proto should already be equal to the protocol of the 8021q
upper, see the call path below.

                           this is the problem
                                   |
skb_vlan_untag()                   v
  -> __vlan_hwaccel_put_tag(skb, skb->protocol, vlan_tci);
    -> skb->vlan_proto = vlan_proto;

But the problem is that skb_vlan_untag() calls __vlan_hwaccel_put_tag
with the wrong vlan_proto, it calls it with the skb->protocol which is
still ETH_P_XDSA because we haven't re-run eth_type_trans() yet.
It looks like this function wants pretty badly to be called after
eth_type_trans(), and it's getting pretty messy because of that, but we
don't have any other driver-specific hook afterwards..

I don't have a lot of experience, the alternatives are either to:
- move dsa_untag_bridge_pvid() after eth_type_trans(), similar to what
  you did in your initial patch - maybe this is the cleanest
- make dsa_untag_bridge_pvid() call eth_type_trans() and this gets rid
  of the extra step you need to do in tag_brcm.c
- document this very well

> Fixes: 412a1526d067 ("net: dsa: untag the bridge pvid from rx skbs")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v2:
> 
> - removed unused list_head iter argument
> 
>  net/dsa/dsa_priv.h | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 0348dbab4131..b4aafb2e90fa 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -205,7 +205,6 @@ static inline struct sk_buff *dsa_untag_bridge_pvid(struct sk_buff *skb)
>  	struct net_device *br = dp->bridge_dev;
>  	struct net_device *dev = skb->dev;
>  	struct net_device *upper_dev;
> -	struct list_head *iter;
>  	u16 vid, pvid, proto;
>  	int err;
>  
> @@ -247,12 +246,10 @@ static inline struct sk_buff *dsa_untag_bridge_pvid(struct sk_buff *skb)
>  	 * supports because vlan_filtering is 0. In that case, we should
>  	 * definitely keep the tag, to make sure it keeps working.
>  	 */
> -	netdev_for_each_upper_dev_rcu(dev, upper_dev, iter) {
> -		if (!is_vlan_dev(upper_dev))
> -			continue;
> -
> -		if (vid == vlan_dev_vlan_id(upper_dev))
> -			return skb;
> +	upper_dev = __vlan_find_dev_deep_rcu(br, htons(proto), vid);
> +	if (upper_dev) {
> +		skb->vlan_proto = vlan_dev_vlan_proto(upper_dev);
> +		return skb;
>  	}
>  
>  	__vlan_hwaccel_clear_tag(skb);
> -- 
> 2.25.1
>

On Fri, Oct 02, 2020 at 02:24:02AM +0300, Vladimir Oltean wrote:
> The explanation is super confusing, although I think the placement of
> the "skb->vlan_proto = vlan_dev_vlan_proto(upper_dev)" is correct.

No, I think it _is_ wrong, after all, I think you're repairing
skb->vlan_proto only for that particular 8021q upper, but not for the
rest. I think the correct approach would be to say "skb->protocol =
hdr->h_vlan_proto" right before calling skb_vlan_untag().

On 10/1/2020 4:24 PM, Vladimir Oltean wrote:
> On Wed, Sep 30, 2020 at 08:06:23PM -0700, Florian Fainelli wrote:
>> The intent of 412a1526d067 ("net: dsa: untag the bridge pvid from rx
>> skbs") is to transparently untag the bridge's default_pvid when the
>> Ethernet switch can only support egress tagged of that default_pvid
>> towards the CPU port.
>>
>> Prior to this commit, users would have to configure an 802.1Q upper on
>> the bridge master device when the bridge is configured with
>> vlan_filtering=0 in order to pop the VLAN tag:
>>
>> ip link add name br0 type bridge vlan_filtering 0
>> ip link add link br0 name br0.1 type vlan id 1
>>
>> After this commit we added support for managing a switch port 802.1Q
>> upper but those are not usually added as bridge members, and if they do,
>> they do not actually require any special management, the data path would
>> pop the desired VLAN tag accordingly.
>>
>> What we want to preserve is that use case and to manage when the user
>> creates that 802.1Q upper for the bridge port.
>>
>> While we are it, call __vlan_find_dev_deep_rcu() which makes use the
>> VLAN group array which is faster.
>>
>> As soon as we return the VLAN tagged SKB though it will be used by the
>> following call path:
>>
>> netif_receive_skb_list_internal
>>    -> __netif_receive_skb_list_core
>>      -> __netif_receive_skb_core
>>        -> vlan_do_receive()
>>
>> which uses skb->vlan_proto, if we do not set it to the appropriate VLAN
>> protocol, we will leave it set to what the DSA master has set
>> (ETH_P_XDSA).
>>
> 
> The explanation is super confusing, although I think the placement of
> the "skb->vlan_proto = vlan_dev_vlan_proto(upper_dev)" is correct.
> Here's what I think is going on. It has to do with what's upwards of the
> code you're changing:
> 
> 	/* Move VLAN tag from data to hwaccel */
> 	if (!skb_vlan_tag_present(skb) && hdr->h_vlan_proto == htons(proto)) {
> 		skb = skb_vlan_untag(skb);
> 		if (!skb)
> 			return NULL;
> 	}
> 
> So skb->vlan_proto should already be equal to the protocol of the 8021q
> upper, see the call path below.
> 
>                             this is the problem
>                                     |
> skb_vlan_untag()                   v
>    -> __vlan_hwaccel_put_tag(skb, skb->protocol, vlan_tci);
>      -> skb->vlan_proto = vlan_proto;

Ah, indeed!

> 
> But the problem is that skb_vlan_untag() calls __vlan_hwaccel_put_tag
> with the wrong vlan_proto, it calls it with the skb->protocol which is
> still ETH_P_XDSA because we haven't re-run eth_type_trans() yet.
> It looks like this function wants pretty badly to be called after
> eth_type_trans(), and it's getting pretty messy because of that, but we
> don't have any other driver-specific hook afterwards..
> 
> I don't have a lot of experience, the alternatives are either to:
> - move dsa_untag_bridge_pvid() after eth_type_trans(), similar to what
>    you did in your initial patch - maybe this is the cleanest

This would be my preference and it would not be hurting the fast-path 
that much.

> - make dsa_untag_bridge_pvid() call eth_type_trans() and this gets rid
>    of the extra step you need to do in tag_brcm.c

Sure, however this requires that we remove the call to eth_type_trans() 
in dsa_switch_rcv() or that we push/pull by an appropriate amount, not 
very effective.

> - document this very well

I doubt this would survive the test of time unfortunately.

> 
>> Fixes: 412a1526d067 ("net: dsa: untag the bridge pvid from rx skbs")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> Changes in v2:
>>
>> - removed unused list_head iter argument
>>
>>   net/dsa/dsa_priv.h | 11 ++++-------
>>   1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
>> index 0348dbab4131..b4aafb2e90fa 100644
>> --- a/net/dsa/dsa_priv.h
>> +++ b/net/dsa/dsa_priv.h
>> @@ -205,7 +205,6 @@ static inline struct sk_buff *dsa_untag_bridge_pvid(struct sk_buff *skb)
>>   	struct net_device *br = dp->bridge_dev;
>>   	struct net_device *dev = skb->dev;
>>   	struct net_device *upper_dev;
>> -	struct list_head *iter;
>>   	u16 vid, pvid, proto;
>>   	int err;
>>   
>> @@ -247,12 +246,10 @@ static inline struct sk_buff *dsa_untag_bridge_pvid(struct sk_buff *skb)
>>   	 * supports because vlan_filtering is 0. In that case, we should
>>   	 * definitely keep the tag, to make sure it keeps working.
>>   	 */
>> -	netdev_for_each_upper_dev_rcu(dev, upper_dev, iter) {
>> -		if (!is_vlan_dev(upper_dev))
>> -			continue;
>> -
>> -		if (vid == vlan_dev_vlan_id(upper_dev))
>> -			return skb;
>> +	upper_dev = __vlan_find_dev_deep_rcu(br, htons(proto), vid);
>> +	if (upper_dev) {
>> +		skb->vlan_proto = vlan_dev_vlan_proto(upper_dev);
>> +		return skb;
>>   	}
>>   
>>   	__vlan_hwaccel_clear_tag(skb);
>> -- 
>> 2.25.1

On Thu, Oct 01, 2020 at 04:48:43PM -0700, Florian Fainelli wrote:
> > - move dsa_untag_bridge_pvid() after eth_type_trans(), similar to what
> >    you did in your initial patch - maybe this is the cleanest
>
> This would be my preference and it would not be hurting the fast-path that
> much.

Ok, let's do that. You can also replace the hdr->h_vlan_proto with
skb->protocol in that case, and remove this:

	struct vlan_ethhdr *hdr = vlan_eth_hdr(skb);

Thanks!
-Vladimir