linux-kernel - Re: [PATCH net v3] net: dsa: mxl-gsw1xx: manually clear RANEG bit

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] [day] [month] [year] [list]

Message-ID: <aTb0IqktR9gbZFdn@shell.armlinux.org.uk>
Date: Mon, 8 Dec 2025 15:52:02 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>
Cc: Daniel Golle <daniel@...rotopia.org>, Hauke Mehrtens <hauke@...ke-m.de>,
	Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Rasmus Villemoes <ravi@...vas.dk>,
	"Benny (Ying-Tsan) Weng" <yweng@...linear.com>,
	John Crispin <john@...ozen.org>
Subject: Re: [PATCH net v3] net: dsa: mxl-gsw1xx: manually clear RANEG bit

On Mon, Dec 08, 2025 at 02:37:38PM +0100, Andrew Lunn wrote:
> On Mon, Dec 08, 2025 at 12:47:51PM +0000, Russell King (Oracle) wrote:
> > On Mon, Dec 08, 2025 at 01:27:04AM +0000, Daniel Golle wrote:
> > >  static void gsw1xx_remove(struct mdio_device *mdiodev)
> > >  {
> > >  	struct gswip_priv *priv = dev_get_drvdata(&mdiodev->dev);
> > > +	struct gsw1xx_priv *gsw1xx_priv;
> > >  
> > >  	if (!priv)
> > >  		return;
> > >  
> > > +	gsw1xx_priv = container_of(priv, struct gsw1xx_priv, gswip);
> > > +	cancel_delayed_work_sync(&gsw1xx_priv->clear_raneg);
> > > +
> > >  	gswip_disable_switch(priv);
> > >  
> > >  	dsa_unregister_switch(priv->ds);
> > 
> > Can we please pay attention to ->remove methods, and code them properly
> > please?
> > 
> > There are two golden rules of driver programming.
> > 
> > 1. Do not publish the device during probe until hardware setup is
> >    complete. If you publish before hardware setup is complete, userspace
> >    is free to race with the hardware setup and start using the device.
> >    This is especially true of recent systems which use hotplug events
> >    via udev and systemd to do stuff.
> > 
> > 2. Do not start tearing down a device until the user interfaces have
> >    been unpublished. Similar to (1), while the user interface is
> >    published, uesrspace is completely free to interact with the device
> >    in any way it sees fit.
> > 
> > In this case, what I'm concerned with is the call above to
> > cancel_delayed_work_sync() before dsa_unregister_switch(). While
> > cancel_delayed_work_sync() will stop this work and wait for the handler
> > to finish running before returning (which is safe) there is a window
> > between this call and dsa_unregister_switch() where the user _could_
> > issue a badly timed ethtool command which invokes
> > gsw1xx_pcs_an_restart(), which would re-schedule the delayed work,
> > thus undoing the cancel_delayed_work_sync() effect in this path.
> 
> And this is why is was pushing for the much simpler msleep(10), or
> io_poll.h polling to see if it self clears. It is hard to get that
> wrong, where as delayed work is much easier to get wrong.

It's not specific to delayed work. Looking at the context around
the ->remove() method, it's already wrong:

        gswip_disable_switch(priv);

        dsa_unregister_switch(priv->ds);

gswip_disable_switch() writes to a register:

	regmap_clear_bits(priv->mdio, GSWIP_MDIO_GLOB, GSWIP_MDIO_GLOB_ENABLE);

and I wonder what that does in terms of MDIO bis accesses that will
still be active at this point (because the DSA switch is still
registered.)

I see that gswip_setup() enables the switch before it configures the
MDIO bus and registers it, so the disable-then-unregister looks very
suspicious to me.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!