netdev - Re: [PATCH net-next v3 08/12] net: dsa: rzn1-a5psw: add FDB 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20220504162457.eeggo4xenvxddpkr@skbuf>
Date:   Wed, 4 May 2022 19:24:57 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Clément Léger <clement.leger@...tlin.com>
Cc:     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>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Magnus Damm <magnus.damm@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        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>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 08/12] net: dsa: rzn1-a5psw: add FDB support

On Wed, May 04, 2022 at 11:29:56AM +0200, Clément Léger wrote:
> +static int a5psw_port_fdb_del(struct dsa_switch *ds, int port,
> +			      const unsigned char *addr, u16 vid,
> +			      struct dsa_db db)
> +{
> +	struct a5psw *a5psw = ds->priv;
> +	union lk_data lk_data = {0};
> +	bool clear = false;
> +	int ret = 0;
> +	u16 entry;
> +	u32 reg;
> +
> +	ether_addr_copy(lk_data.entry.mac, addr);
> +
> +	spin_lock(&a5psw->lk_lock);
> +
> +	ret = a5psw_lk_execute_lookup(a5psw, &lk_data, &entry);
> +	if (ret) {
> +		dev_err(a5psw->dev, "Failed to lookup mac address\n");
> +		goto lk_unlock;
> +	}
> +
> +	lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI);
> +	if (!lk_data.entry.valid) {
> +		dev_err(a5psw->dev, "Tried to remove non-existing entry\n");
> +		ret = -ENOENT;
> +		goto lk_unlock;

These error messages can happen under quite normal use with your hardware.
For example:

ip link add br0 type bridge && ip link set br0 master br0
bridge fdb add dev swp0 00:01:02:03:04:05 vid 1 master static
bridge fdb add dev swp0 00:01:02:03:04:05 vid 2 master static
bridge fdb del dev swp0 00:01:02:03:04:05 vid 2 master static
bridge fdb del dev swp0 00:01:02:03:04:05 vid 1 master static

Because the driver ignores the VID, then as soon as the VID 2 FDB entry
is removed, the VID 1 FDB entry doesn't exist anymore, either.

The above is a bit artificial. More practically, the bridge installs
local FDB entries (MAC address of bridge device, MAC addresses of ports)
once in the VLAN-unaware database (VID 0), and once per VLAN installed
on br0.

When the MAC address of br0 is different from that of the ports, and it
is changed by the user, you'll get these errors too, since those changes
translate into a deletion of the old local FDB entries (installed as FDB
entries towards the CPU port). Do you want the users to see them and
think something is wrong?  I mean, something _is_ wrong (the hardware),
but you have to work with that somehow.

> +	}
> +
> +	lk_data.entry.port_mask &= ~BIT(port);
> +	/* If there is no more port in the mask, clear the entry */
> +	if (lk_data.entry.port_mask == 0)
> +		clear = true;
> +
> +	a5psw_reg_writel(a5psw, A5PSW_LK_DATA_HI, lk_data.hi);
> +
> +	reg = entry;
> +	if (clear)
> +		reg |= A5PSW_LK_ADDR_CTRL_CLEAR;
> +	else
> +		reg |= A5PSW_LK_ADDR_CTRL_WRITE;
> +
> +	ret = a5psw_lk_execute_ctrl(a5psw, &reg);
> +	if (ret)
> +		goto lk_unlock;
> +
> +	/* Decrement LEARNCOUNT */
> +	if (clear) {
> +		reg = A5PSW_LK_LEARNCOUNT_MODE_DEC;
> +		a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg);
> +	}
> +
> +lk_unlock:
> +	spin_unlock(&a5psw->lk_lock);
> +
> +	return ret;
> +}