Re: [PATCH] driver core: complete deferred binds when drivers_autoprobe is off

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] driver core: TOCTOU race in device_initial_probe() causes spurious autoprobe
--

commit 363268c1e7ac2e5ecc70b8f7c4762aa363c48d73
Author: Mohammad Mosafer <mohsafer@gmail.com>

driver core: complete deferred binds when drivers_autoprobe is off

This commit updates device_initial_probe() to complete deferred device binds
even when bus autoprobing is disabled. It conditionally calls
__device_attach() if dev->driver is already set, preventing a null-pointer
dereference during device teardown.

> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index f6525a7ee8c54..98315264295e4 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -1149,7 +1149,17 @@ void device_initial_probe(struct device *dev)
>  	if (!sp)
>  		return;
>  
> -	if (sp->drivers_autoprobe)
> +	/*
> +	 * The drivers_autoprobe flag only suppresses matching the device
> +	 * against the bus drivers.  If a driver has already been assigned
> +	 * to the device (e.g. usb_driver_claim_interface() pre-setting
> +	 * dev->driver for an unregistered interface, expecting device_add()
> +	 * to complete the bind), the bind must be finished regardless of
> +	 * autoprobe: otherwise the device ends up registered with
> +	 * dev->driver set but never bound, and teardown oopses removing
> +	 * the never-attached knode_driver from the driver klist.
> +	 */
> +	if (sp->drivers_autoprobe || dev->driver)
>  		__device_attach(dev, true);

[Severity: Medium]
If sp->drivers_autoprobe is 0 but dev->driver is set, device_initial_probe()
locklessly evaluates this as true and calls __device_attach().

Before __device_attach() acquires device_lock(dev), what happens if a
concurrent operation (like a sysfs unbind or sibling device disconnect)
acquires the lock, detaches the driver, and sets dev->driver to NULL?

When __device_attach() subsequently acquires the lock, it would check
dev->driver. Finding it NULL, it would skip the pre-assigned bind logic and
fall through to the else block:

static int __device_attach(struct device *dev, bool allow_async)
{
	...
	device_lock(dev);
	if (dev->p->dead) {
		goto out_unlock;
	} else if (dev->driver) {
		...
	} else {
		...
		ret = bus_for_each_drv(dev->bus, NULL, &data,
					__device_attach_driver);

This else block unconditionally executes bus_for_each_drv(), which performs
a full automatic match and probe against all drivers on the bus.

Does this result in a spurious bus-wide autoprobe, directly violating the
administrator's explicit drivers_autoprobe = 0 policy?

>  
>  	subsys_put(sp);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008154105.256340-1-mosafer@node0.quickhttpnode15.cloudfaas-pg0.wisc.cloudlab.us?part=1