netdev - Re: [PATCH RESEND net-next v4 3/3] net: dsa: rzn1-a5psw: add vlan support

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20230330110959.2132cd07@fixe.home>
Date:   Thu, 30 Mar 2023 11:09:59 +0200
From:   Clément Léger <clement.leger@...tlin.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Herve Codina <herve.codina@...tlin.com>,
        Miquèl Raynal <miquel.raynal@...tlin.com>,
        Milan Stevanovic <milan.stevanovic@...com>,
        Jimmy Lalande <jimmy.lalande@...com>,
        Pascal Eberhard <pascal.eberhard@...com>,
        Arun Ramadoss <Arun.Ramadoss@...rochip.com>,
        linux-renesas-soc@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Alexis Lothore <alexis.lothore@...tlin.com>
Subject: Re: [PATCH RESEND net-next v4 3/3] net: dsa: rzn1-a5psw: add vlan
 support

Le Wed, 29 Mar 2023 16:16:13 +0300,
Vladimir Oltean <olteanv@...il.com> a écrit :

> > After thinking about the current mechasnim, let me summarize why I
> > think it almost matches what you described in this last paragraph:
> > 
> > - Port is set to match a specific matching rule which will enforce port
> >   to CPU forwarding only based on the MGMTFWD bit of PATTERN_CTRL which
> >   states the following: "When set, the frame is forwarded to the
> >   management port only (suppressing destination address lookup)"
> > 
> > This means that for the "port to CPU" path when in standalone mode, we
> > are fine. Regarding the other "CPU to port" path only:
> > 
> > - Learning will be disabled when leaving the bridge. This will allow
> >   not to have any new forwarding entries in the MAC lookup table.
> > 
> > - Port is fast aged which means it won't be targeted for packet
> >   forwarding.
> > 
> > - We remove the port from the flooding mask which means it won't be
> >   flooded after being removed from the port.
> > 
> > Based on that, the port should not be the target of any forward packet
> > from the other ports. Note that anyway, even if using per-port VLAN for
> > standalone mode, we would also end up needing to disable learning,
> > fast-age the port and disable flooding (at least from my understanding
> > if we want the port to be truly isolated).
> > 
> > Tell me if it makes sense.  
> 
> This makes sense.
> 
> However, I still spotted a bug and I don't know where to mention it
> better, so I'll mention it here:
> 
> a5psw_port_vlan_add()
> 
> 	if (pvid) {
> 		a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port),
> 			      BIT(port));
> 		a5psw_reg_writel(a5psw, A5PSW_SYSTEM_TAGINFO(port), vid);
> 	}
> 
> You don't want a5psw_port_vlan_add() to change VLAN_IN_MODE_ENA, because
> port_vlan_add() will be called even for VLAN-unaware bridges, and you
> want all traffic to be forwarded as if untagged, and not according to
> the PVID. In other words, in a setup like this:
> 
> ip link add br0 type bridge vlan_filtering 0 && ip link set br0 up
> ip link set swp0 master br0 && ip link set swp0 up
> ip link set swp1 master br0 && ip link set swp1 up
> bridge vlan del dev swp1 vid 1
> 
> forwarding should still take place with no issues, because the entire
> VLAN table is bypassed by the software bridge when vlan_filtering=0, and
> the hardware accelerator should replicate that behavior.

Ok, we'll see how to fix that.

> 
> I suspect that the PVID handling in a5psw_port_vlan_del() is also
> incorrect:
> 
> 	/* Disable PVID if the vid is matching the port one */
> 	if (vid == a5psw_reg_readl(a5psw, A5PSW_SYSTEM_TAGINFO(port)))
> 		a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port), 0);
> 
> VLAN-aware bridge ports without a PVID should drop untagged and VID-0-tagged
> packets. However, as per your own comments:
> 
> | > What does it mean to disable PVID?
> | 
> | It means it disable the input tagging of packets with this PVID.
> | Incoming packets will not be modified and passed as-is.
> 
> so this is not what happens.

Yes indeed, and we noticed the handling of VLANVERI and VLANDISC in
vlan_filtering() should be set according to the fact there is a PVID or
not (which is not the case right now).

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com