linux-kernel - Re: [Patch RFC net-next 4/4] net: dsa: microchip: use private pvid for 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: <20220802105935.gjjc3ft6zf35xrhr@skbuf>
Date:   Tue, 2 Aug 2022 13:59:35 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Arun Ramadoss <arun.ramadoss@...rochip.com>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Woojung Huh <woojung.huh@...rochip.com>,
        UNGLinuxDriver@...rochip.com, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        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>,
        Russell King <linux@...linux.org.uk>
Subject: Re: [Patch RFC net-next 4/4] net: dsa: microchip: use private pvid
 for bridge_vlan_unwaware

On Fri, Jul 29, 2022 at 08:47:33PM +0530, Arun Ramadoss wrote:
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 516fb9d35c87..8a5583b1f2f4 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -161,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
>  	.vlan_filtering = ksz8_port_vlan_filtering,
>  	.vlan_add = ksz8_port_vlan_add,
>  	.vlan_del = ksz8_port_vlan_del,
> +	.drop_untagged = ksz8_port_enable_pvid,

You'll have to explain this one. What impact does PVID insertion on KSZ8
have upon dropping/not dropping untagged packets? This patch is saying
that when untagged packets should be dropped, PVID insertion should be
enabled, and when untagged packets should be accepted, PVID insertion
should be disabled. How come?

>  	.mirror_add = ksz8_port_mirror_add,
>  	.mirror_del = ksz8_port_mirror_del,
>  	.get_caps = ksz8_get_caps,
> @@ -187,6 +188,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
>  	.vlan_filtering = ksz9477_port_vlan_filtering,
>  	.vlan_add = ksz9477_port_vlan_add,
>  	.vlan_del = ksz9477_port_vlan_del,
> +	.drop_untagged = ksz9477_port_drop_untagged,
>  	.mirror_add = ksz9477_port_mirror_add,
>  	.mirror_del = ksz9477_port_mirror_del,
>  	.get_caps = ksz9477_get_caps,
> @@ -220,6 +222,7 @@ static const struct ksz_dev_ops lan937x_dev_ops = {
>  	.vlan_filtering = ksz9477_port_vlan_filtering,
>  	.vlan_add = ksz9477_port_vlan_add,
>  	.vlan_del = ksz9477_port_vlan_del,
> +	.drop_untagged = ksz9477_port_drop_untagged,
>  	.mirror_add = ksz9477_port_mirror_add,
>  	.mirror_del = ksz9477_port_mirror_del,
>  	.get_caps = lan937x_phylink_get_caps,
> @@ -1254,6 +1257,9 @@ static int ksz_enable_port(struct dsa_switch *ds, int port,
>  {
>  	struct ksz_device *dev = ds->priv;
>  
> +	dev->dev_ops->vlan_add(dev, port, KSZ_DEFAULT_VLAN,
> +			       BRIDGE_VLAN_INFO_UNTAGGED);
> +

How many times can this be executed before the VLAN add operation fails
due to the VLAN already being present on the port? I notice you're
ignoring the return code. Wouldn't it be better to do this at
port_setup() time instead?

(side note, the PVID for standalone mode can be added at port_setup
time. The PVID to use for bridges in VLAN-unaware mode can be allocated
at port_bridge_join time)

>  	if (!dsa_is_user_port(ds, port))
>  		return 0;
>  
> +static int ksz_commit_pvid(struct dsa_switch *ds, int port)
> +{
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct net_device *br = dsa_port_bridge_dev_get(dp);
> +	struct ksz_device *dev = ds->priv;
> +	u16 pvid = KSZ_DEFAULT_VLAN;
> +	bool drop_untagged = false;
> +	struct ksz_port *p;
> +
> +	p = &dev->ports[port];
> +
> +	if (br && br_vlan_enabled(br)) {
> +		pvid = p->bridge_pvid.vid;
> +		drop_untagged = !p->bridge_pvid.valid;
> +	}

This is better in the sense that it resolves the need for the
configure_vlan_while_not_filtering hack. But standalone and VLAN-unaware
bridge ports still share the same PVID. Even more so, standalone ports
have address learning enabled, which will poison the address database of
VLAN-unaware bridge ports (and of other standalone ports):
https://patchwork.kernel.org/project/netdevbpf/patch/20220802002636.3963025-1-vladimir.oltean@nxp.com/

Are you going to do further work in this area?

> +
> +	ksz_set_pvid(dev, port, pvid);
> +
> +	if (dev->dev_ops->drop_untagged)
> +		dev->dev_ops->drop_untagged(dev, port, drop_untagged);
> +
> +	return 0;
> +}