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

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

On Thu Aug 20, 2026 at 7:22 PM CEST, Alan Stern wrote:
> On Thu, Aug 20, 2026 at 06:14:37PM +0200, Danilo Krummrich wrote:
>> (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.
>
> I don't object to the idea of adding a "claimed" field to usb_interface.  
>
> However, isn't it true that the driver core has always supported the 
> idea of a driver being associated with a device before binding?  In 
> particular, __device_attach() specifically checks for dev->driver being 
> already set.  If it is, the match and probe steps are skipped.

Ah, I also wanted to add a comment about this, but forgot about it, sorry.

This was added ~25 years ago to be able to hardwire a system device to a
specific driver, which less than a year after was replaced entirely by something
else and later converted back to a bus with proper match() logic. But the
dev->driver special case in __device_attach() was never removed.

So, I guess it is fair to say it was always there, but the driver core does not
really handle dev->driver being set "randomly" outside of the bind/unbind
lifecycle in general. It is expected that it is only set when the driver is
actually bound (or binding).

One example is __device_release_driver(), if device_bind_driver() (or
really_probe()) wasn't called, we should also not land within the
if (dev->driver) conditional of __device_release_driver().

Also note that usb_driver_claim_interface() is the only callsite in the kernel
that does set dev->driver without a subsequent device_bind_driver() while
holding the device lock.

(Well, actually, after a thorough check, there's w1, which hardwires the
w1_slave_driver, where it should actually use a trivial match() callback.)

In general, matching should be done through the match() callback.

> Are you saying that this code path should be removed as well?

Yes, I think we should remove it. It's a rather error prone special case that
can easily be handled with normal match() logic.