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

[Paolo Abeni <pabeni@redhat.com>]

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.