netdev - Re: [PATCH net] net: dsa: reference count the host mdb addresses

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: <20201020000632.GQ456889@lunn.ch>
Date:   Tue, 20 Oct 2020 02:06:32 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, kuba@...nel.org, f.fainelli@...il.com,
        vivien.didelot@...il.com
Subject: Re: [PATCH net] net: dsa: reference count the host mdb addresses

On Fri, Oct 16, 2020 at 12:27:11AM +0300, Vladimir Oltean wrote:
> Currently any DSA switch that implements the multicast ops (properly,
> that is) gets these errors after just sitting for a while, with at least
> 2 ports bridged:
> 
> [  286.013814] mscc_felix 0000:00:00.5 swp3: failed (err=-2) to del object (id=3)
> 
> The reason has to do with this piece of code:
> 
> 	netdev_for_each_lower_dev(dev, lower_dev, iter)
> 		br_mdb_switchdev_host_port(dev, lower_dev, mp, type);
> 
> called from:
> 
> br_multicast_group_expired
> -> br_multicast_host_leave
>    -> br_mdb_notify
>       -> br_mdb_switchdev_host
> 
> Basically, that code is correct. It tells each switchdev port that the
> host can leave that multicast group. But in the case of DSA, all user
> ports are connected to the host through the same pipe. So, because DSA
> translates a host MDB to a normal MDB on the CPU port, this means that
> when all user ports leave a multicast group, DSA tries to remove it N
> times from the CPU port.

Hi Vladimir

I agree with the analysis. This is how i designed it!

As far as i remember, none of the switches at the time would report an
error when asked to delete an MDB which did not exist. They also would
not give an error when adding an MDB which already exists.

So i decided to keep it KISS and not bother with reference counting.

> +static int dsa_host_mdb_add(struct dsa_port *dp,
> +			    const struct switchdev_obj_port_mdb *mdb,
> +			    struct switchdev_trans *trans)
> +{
> +	struct dsa_port *cpu_dp = dp->cpu_dp;
> +	struct dsa_switch *ds = dp->ds;
> +	struct dsa_host_addr *a;
> +	int err;
> +
> +	/* Only the commit phase is refcounted, which means that for the
> +	 * second, third, etc port which is member of this host address,
> +	 * we'll call the prepare phase but never commit.
> +	 */
> +	if (switchdev_trans_ph_prepare(trans))
> +		return dsa_port_mdb_add(cpu_dp, mdb, trans);
> +
> +	a = dsa_host_mdb_find(ds, mdb);
> +	if (a) {
> +		refcount_inc(&a->refcount);
> +		return 0;
> +	}
> +
> +	a = kzalloc(sizeof(*a), GFP_KERNEL);
> +	if (!a)
> +		return -ENOMEM;
> +

The other part of the argument is that DSA is stateless, in that there
is no dynamic memory allocation. Drivers are also stateless in terms
of dynamically allocating memory.

So, what value do this code add? Why do we actually need reference
counting?

	Andrew