Re: [PATCH net-next v10 3/5] net: dsa: mxl862xx: add devlink flash_update and info_get

[Daniel Golle <daniel@makrotopia.org>]

On Thu, Aug 13, 2026 at 10:22:33AM +0200, Paolo Abeni wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev/
> ---
> Subject: net: dsa: mxl862xx: add devlink flash_update and info_get
> 
> This commit implements runtime firmware upgrading for the MxL862xx switch
> using devlink dev flash. It transfers the firmware image over SB PDI,
> reboots the switch, and defers a device reprobe to re-initialize the driver
> using a workqueue.
> 
> > diff --git a/drivers/net/dsa/mxl862xx/mxl862xx-fw.c b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c
> > new file mode 100644
> > index 0000000000000..9799cf19d31a9
> > --- /dev/null
> > +++ b/drivers/net/dsa/mxl862xx/mxl862xx-fw.c
> 
> [ ... ]
> 
> > +static void mxl862xx_reprobe_work_fn(struct work_struct *work)
> > +{
> > +	struct mxl862xx_reprobe *rp =
> > +		container_of(work, struct mxl862xx_reprobe, work.work);
> > +	struct device *dev = rp->dev;
> > +	bool bound;
> > +
> > +	device_lock(dev);
> > +	bound = !!dev_get_drvdata(dev);
> > +	device_unlock(dev);
> > +
> > +	if (!bound)
> > +		dev_dbg(dev, "reprobe skipped, device is gone\n");
> > +	else if (device_reprobe(dev))
> 
> [Severity: High]
> Is there a race window here that can lead to a use-after-free of priv?
> 
> If device_shutdown() runs after device_unlock() but before
> device_reprobe(), it can clear drvdata. When device_reprobe() subsequently
> runs, it triggers the driver remove() function, which returns early because
> drvdata is NULL, bypassing dsa_unregister_switch().
> 
> However, because priv was allocated with devm_kzalloc() during probe:
> 
> drivers/net/dsa/mxl862xx/mxl862xx.c:mxl862xx_probe() {
>     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>     ...
> }
> 
> the driver core will still run devres_release_all() and free priv. The DSA
> switch remains registered, and any subsequent access to it via devlink or
> netlink could dereference the freed priv pointer.
> 
> > +		dev_err(dev, "reprobe failed; device left unbound\n");
> > +
> > +	put_device(dev);
> > +	kfree(rp);
> > +	module_put(THIS_MODULE);
> > +}
> 
> [Severity: High]
> Can this cause a kernel panic if the module is unloaded concurrently?
> 
> Calling module_put(THIS_MODULE) at the very end of a work item means the
> module reference count drops to zero before the function fully returns to
> the workqueue core.
> 
> A concurrent rmmod can then proceed and unmap the module text section. If
> the worker thread is preempted or delayed, it will resume execution from
> unmapped memory during the function epilogue, leading to a fatal page fault.
> -- 
> This is an AI-generated review.
> 

Both issues are real, but hard to trigger in practise.
See

https://lore.kernel.org/all/anpxFdwNxk0XwPjQ@makrotopia.org/

for a proposal fixing this in other existing drivers (mxl862xx re-probe is
modelled after iwlwifi which suffers from the same issue)

I suggest to merge this series as is, I'll then convert mxl862xx to use
device_schedule_reprobe() once it has landed.