Re: [PATCH v4] driver core: avoid klist_remove() on unattached knode_driver

["Danilo Krummrich" <dakr@kernel.org>]

(Cc: linux-usb)

On Thu Aug 20, 2026 at 10:45 AM CEST, Nguyen Quang Le Kien wrote:
> Fixes: 94e7b1c5ff20 ("[PATCH] Add a klist to struct device_driver for the devices bound to it.")

This is not the correct commit to reference, this commit seems fine.

> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 60c005223..4154b4499 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -1354,7 +1354,8 @@ static void __device_release_driver(struct device *dev, struct device *parent)
>  		device_unbind_cleanup(dev);
>  		device_links_driver_cleanup(dev);
>  
> -		klist_remove(&dev->p->knode_driver);
> +		if (device_is_bound(dev))
> +			klist_remove(&dev->p->knode_driver);

This looks like band-aid for the underlying design tension in the USB core
(which we should address instead) and does not belong in the driver core.

It's not visible from the above diff, but with this patch the control flow
becomes:

	if (dev->driver) {
		if (device_is_bound(dev))
			klist_remove(&dev->p->knode_driver);
		...
	}

but dev->driver already indicates that the device is bound to dev->driver in
this context.

The reason we "need" this check regardless is that usb_driver_claim_interface()
(ab)uses dev->driver to indicate that a certain USB driver claimed, or rather
reserved, this device.

There are two cases in usb_driver_claim_interface():

  (1) The device to claim is already registered with the driver core, in which
      case device_bind_driver(dev) is called and dev->driver is correctly set
      during the bind attempt.

  (2) The device to claim was not yet registered with the driver core. This can
      happen when the USB interface the driver actually binds to is registered
      (and hence probed) before the additional interface the driver wants to
      claim is registered. The USB core sets dev->driver independent of the bind
      state to indicate it has reserved the interface.

The first case is perfectly fine, but the issue with the second case is that now
dev->driver is semantically overloaded:

The USB core treats it as "dev is reserved for dev->driver" and the driver core
treats it as "dev is currently binding or bound to dev->driver", but that's not
actually the case yet, since device_bind_driver() hasn't been called yet.

IOW, dev->driver should only be set under the device lock before calling
device_bind_driver(), and, in case of failure, should be cleared after
device_bind_driver() with the device lock still held.

Besides the reported crash, another implication of this is that all other
functions from device_release_driver() are called as well, even though
device_bind_driver() was never called before, and there is no guarantee that
this does not cause other unexpected side effects already or in the future.

I think one solution could be to add a new claimed field to struct usb_interface
to indicate that the interface is reserved for a certain driver and make
usb_device_match() reject the device if ever probed otherwise.

Another solution (but that's a bit more work) would be to separate interface
registration from interface probing in usb_set_configuration(). Of course that
needs help from the driver core as well, but it would also get us rid of the
slightly odd situation that a driver may operate a claimed device already, even
though it is not yet registered with the driver core.

I'd suggest going for a claimed field in struct usb_interface first to fix the
immediate problem and then take it from there.

That said, I wonder if there's more to think about with the
usb_driver_claim_interface() / usb_driver_release_interface() API.

After having a brief look it seems that drivers have invented various different
approaches to protect against the case where userspace could write the claimed
interface's name to:

	/sys/bus/usb/drivers/<driver>/unbind

I think this could be much cleaner if the driver core would support "claimed"
devices" natively. OTH, there's only ~20 drivers across USB and PnP though, so
probably not worth.