linux-kernel - Re: [net-next,v11,4/4] net: dsa: add basic initial driver for MxL862xx switches

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: <aX6keghdVMI-A6Fq@makrotopia.org>
Date: Sun, 1 Feb 2026 00:55:22 +0000
From: Daniel Golle <daniel@...rotopia.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: lxu@...linear.com, hkallweit1@...il.com, yweng@...linear.com,
	ajayaraman@...linear.com, andrew@...n.ch, netdev@...r.kernel.org,
	bxu@...linear.com, olteanv@...il.com, krzk+dt@...nel.org,
	linux-kernel@...r.kernel.org, lrosu@...linear.com, chad@...roe.io,
	conor+dt@...nel.org, devicetree@...r.kernel.org, robh@...nel.org,
	edumazet@...gle.com, pabeni@...hat.com, cezary.wilmanski@...ran.com,
	davem@...emloft.net, john@...ozen.org, frankwu@....de,
	jpovazanec@...linear.com, linux@...linux.org.uk,
	fchan@...linear.com, horms@...nel.org
Subject: Re: [net-next,v11,4/4] net: dsa: add basic initial driver for
 MxL862xx switches

On Sat, Jan 31, 2026 at 09:52:44AM -0800, Jakub Kicinski wrote:
> > [...]
> > +/**
> > + * struct mxl862xx_bridge_port_alloc - Bridge Port Allocation
> > + * @bridge_port_id: If the bridge port allocation is successful, a valid ID
> > + *                  will be returned in this field. Otherwise, INVALID_HANDLE
> > + *                  is returned. For bridge port free, this field should
> > + *                  contain a valid ID returned by the bridge port allocation.
> > + *
> > + * Used by MXL862XX_BRIDGE_PORT_ALLOC and MXL862XX_BRIDGE_PORT_FREE.
> > + */
> > +struct mxl862xx_bridge_port_alloc {
> > +	__le16 bridge_port_id;
> > +};
> 
> Is the __packed attribute missing here? The similar struct
> mxl862xx_bridge_alloc has __packed, and both structures have identical
> layout with a single __le16 field. Both are used for firmware
> communication where padding could break the protocol.
> 
> [Jakub] please remove the __packed attribute on structs which are
>         naturally packed according to C rules.

I'll do that for the upcoming submission of v12, but wonder if using
'#pragma pack(push, 1)' around the complete API header file wouldn't be
the better option, as anyway *everything* should always be packed there,
as the switch firmware expects data byte-aligned.

> > [...]
> > +static int mxl862xx_setup_mdio(struct dsa_switch *ds)
> > +{
> > +	struct mxl862xx_priv *priv = ds->priv;
> > +	struct device *dev = ds->dev;
> > +	struct device_node *mdio_np;
> > +	struct mii_bus *bus;
> > +	static int idx;
> > +	int ret;
> > +
> > +	bus = devm_mdiobus_alloc(dev);
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	bus->priv = priv;
> > +	ds->user_mii_bus = bus;
> > +	bus->name = KBUILD_MODNAME "-mii";
> > +	snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++);
> 
> Can concurrent probes race on the idx variable?
> 
> The static int idx is shared across all driver instances and accessed
> without synchronization. If multiple instances of this driver are probed
> concurrently on different CPU cores, they could race on the idx++
> operation potentially leading to duplicate MII bus IDs. MII bus IDs must
> be unique system-wide.
> 

Do you think it's worth to fix this also in other drivers which follow
the same pattern? (bcm_sf2, mt7530 and mv88e6xxx if OF node is NULL --
if there is an OF node it uses %pOF, and I'm not sure that's such a good
idea either)

Apart from being racy, this also means the MII bus name depends on probe
order which isn't very nice either. Using

snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(priv->dev));

may result in a slightly more ugly-looking, but predictable and
non-racy (ie. always unique) name.

Other than that there is also
snprintf(bus->id, MII_BUS_ID_SIZE, "%s-%d.%d",
         KBUILD_MODNAME, ds->dst->index, ds->index);

The real question here is if we consider MII bus names to be ABI,
ie. if they should never change once a driver is part of the kernel.