netdev - Re: [PATCH net] dsa: tag_dsa: Handle !CONFIG_BRIDGE_VLAN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211003210354.tiyaqsdje6ju7arz@skbuf>
Date:   Sun, 3 Oct 2021 21:03:55 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     netdev <netdev@...r.kernel.org>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net] dsa: tag_dsa: Handle !CONFIG_BRIDGE_VLAN_FILTERING
 case

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@...n.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.