[PATCH net] dsa: tag_dsa: Handle !CONFIG_BRIDGE_VLAN_FILTERING case

[PATCH net] dsa: tag_dsa: Handle !CONFIG_BRIDGE_VLAN_FILTERING case

[Andrew Lunn]

If CONFIG_BRIDGE_VLAN_FILTERING is disabled, br_vlan_enabled() is
replaced with a stub which returns -EINVAL. As a result the tagger
drops the frame. Rather than drop the frame, use a pvid of 0.

Fixes: d82f8ab0d874 ("net: dsa: tag_dsa: offload the bridge forwarding process")
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/tag_dsa.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index e5127b7d1c6a..8daa8b7787c0 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -149,7 +149,8 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
 		 * inject packets to hardware using the bridge's pvid, since
 		 * that's where the packets ingressed from.
 		 */
-		if (!br_vlan_enabled(br)) {
+		if (IS_ENABLED(CONFIG_BRIDGE_VLAN_FILTERING) &&
+		    !br_vlan_enabled(br)) {
 			/* Safe because __dev_queue_xmit() runs under
 			 * rcu_read_lock_bh()
 			 */
-- 
2.33.0

Re: [PATCH net] dsa: tag_dsa: Handle !CONFIG_BRIDGE_VLAN_FILTERING case

[Vladimir Oltean]

On Sun, Oct 03, 2021 at 05:51:41PM +0200, Andrew Lunn wrote:
> If CONFIG_BRIDGE_VLAN_FILTERING is disabled, br_vlan_enabled() is
> replaced with a stub which returns -EINVAL.

br_vlan_enabled() returns bool, so it cannot hold -EINVAL. The stub for
that returns false. We negate that false, make it true, and then call
br_vlan_get_pvid_rcu() which returns -EINVAL because of _its_ stub
implementation.

> As a result the tagger
> drops the frame. Rather than drop the frame, use a pvid of 0.
> 
> Fixes: d82f8ab0d874 ("net: dsa: tag_dsa: offload the bridge forwarding process")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  net/dsa/tag_dsa.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
> index e5127b7d1c6a..8daa8b7787c0 100644
> --- a/net/dsa/tag_dsa.c
> +++ b/net/dsa/tag_dsa.c
> @@ -149,7 +149,8 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>  		 * inject packets to hardware using the bridge's pvid, since
>  		 * that's where the packets ingressed from.
>  		 */
> -		if (!br_vlan_enabled(br)) {
> +		if (IS_ENABLED(CONFIG_BRIDGE_VLAN_FILTERING) &&
> +		    !br_vlan_enabled(br)) {
>  			/* Safe because __dev_queue_xmit() runs under
>  			 * rcu_read_lock_bh()
>  			 */
> -- 
> 2.33.0
> 

So this got me thinking. We shouldn't behave differently when VLAN
filtering is disabled vs when it is disabled and compiled out.

In fact it is actually wrong to inject into the switch using the
bridge's pvid, if VLAN awareness is turned off. We should be able to
send and receive packets in that mode regardless of whether a pvid
exists for the bridge device or not. That is also what we document in
Documentation/networking/switchdev.rst.

So if VLAN 0 does that trick, perfect, we should just delete the entire
"if (!br_vlan_enabled(br))" block.

Re: [PATCH net] dsa: tag_dsa: Handle !CONFIG_BRIDGE_VLAN_FILTERING case

[Andrew Lunn]

On Sun, Oct 03, 2021 at 09:03:55PM +0000, Vladimir Oltean wrote:
> On Sun, Oct 03, 2021 at 05:51:41PM +0200, Andrew Lunn wrote:
> > If CONFIG_BRIDGE_VLAN_FILTERING is disabled, br_vlan_enabled() is
> > replaced with a stub which returns -EINVAL.
> 
> br_vlan_enabled() returns bool, so it cannot hold -EINVAL. The stub for
> that returns false. We negate that false, make it true, and then call
> br_vlan_get_pvid_rcu() which returns -EINVAL because of _its_ stub
> implementation.

Yeh, i got the names of the functions wrong. I will fix that.

> In fact it is actually wrong to inject into the switch using the
> bridge's pvid, if VLAN awareness is turned off. We should be able to
> send and receive packets in that mode regardless of whether a pvid
> exists for the bridge device or not. That is also what we document in
> Documentation/networking/switchdev.rst.
> 
> So if VLAN 0 does that trick, perfect, we should just delete the entire
> "if (!br_vlan_enabled(br))" block.

I will rework the patch and test it without the if.

Thanks
	Andrew

Re: [PATCH net] dsa: tag_dsa: Handle !CONFIG_BRIDGE_VLAN_FILTERING case

[Vladimir Oltean]

On Mon, Oct 04, 2021 at 02:18:54PM +0200, Andrew Lunn wrote:
> On Sun, Oct 03, 2021 at 09:03:55PM +0000, Vladimir Oltean wrote:
> > On Sun, Oct 03, 2021 at 05:51:41PM +0200, Andrew Lunn wrote:
> > > If CONFIG_BRIDGE_VLAN_FILTERING is disabled, br_vlan_enabled() is
> > > replaced with a stub which returns -EINVAL.
> > 
> > br_vlan_enabled() returns bool, so it cannot hold -EINVAL. The stub for
> > that returns false. We negate that false, make it true, and then call
> > br_vlan_get_pvid_rcu() which returns -EINVAL because of _its_ stub
> > implementation.
> 
> Yeh, i got the names of the functions wrong. I will fix that.
> 
> > In fact it is actually wrong to inject into the switch using the
> > bridge's pvid, if VLAN awareness is turned off. We should be able to
> > send and receive packets in that mode regardless of whether a pvid
> > exists for the bridge device or not. That is also what we document in
> > Documentation/networking/switchdev.rst.
> > 
> > So if VLAN 0 does that trick, perfect, we should just delete the entire
> > "if (!br_vlan_enabled(br))" block.
> 
> I will rework the patch and test it without the if.
> 
> Thanks
> 	Andrew

I already resent the patch here:
https://patchwork.kernel.org/project/netdevbpf/patch/20211003222312.284175-2-vladimir.oltean@nxp.com/