Re: [PATCH] driver core: take parent lock on deferred probe

[Johan Hovold <johan@kernel.org>]

On Tue, Oct 13, 2026 at 11:01:16PM +0200, Danilo Krummrich wrote:
> On Tue Oct 13, 2026 at 9:11 PM CEST, Alan Stern wrote:
> > On Tue, Oct 13, 2026 at 07:43:08PM +0200, Danilo Krummrich wrote:
> >> On Tue Oct 13, 2026 at 4:06 PM CEST, Johan Hovold wrote:
> >> > +void bus_probe_device_deferred(struct device *dev)
> >> > +{
> >> > +	if (dev->parent && dev->bus->need_parent_lock)
> >> > +		device_lock(dev->parent);
> >> 
> >> This looks like it has the same potential UAF as the other patch.  It's quite
> >> unfortunate we need the whole need_parent_lock thing just for USB.  IIUC the
> >> main reason USB needs this is because interface devices share resources of USB
> >> devices and we need to keep the locking order to avoid inversions, right?  Is
> >> there more to this from the USB side of things?
> >
> > That's pretty much it.
> >
> >> I think we can probably do something like this
> >> 
> >> 	struct device *parent = NULL;
> >> 
> >> 	scoped_guard(device, dev) {
> >> 		if (dev->p->dead)
> >> 			return;
> >> 	
> >> 		if (dev->bus->need_parent_lock)
> >> 			parent = get_device(dev->parent);
> >> 	}
> >> 
> >> to avoid the problem, but it's still a bit ugly.
> >> 
> >> > +	__bus_probe_device(dev);
> >> > +	if (dev->parent && dev->bus->need_parent_lock)
> >> > +		device_unlock(dev->parent);
> >> > +}
> >
> > Wouldn't it be simpler just to make each USB interface take a reference 
> > to its parent device?  And likewise for USB devices, if needed?
> 
> The driver core already holds a reference count of the parent between
> device_add() and device_del(), which is where a "parent" is technically defined.
> But the deferred retry can extend beyond device_del().

Yeah, but we could take a reference also to the parent under the
deferred_probe_mutex in deferred_probe_work_func().

And similarly, the device should be registered when __device_attach() is
called so that we can take another parent reference for async probing.

We do however have this bug already today for some other cases.
For example, bus_add_driver() loops over all devices on the bus, but
only checks if the device is dead after __driver_attach() has already
taken the parent lock.

> USB interfaces already take an additional reference to their USB device in
> usb_set_configuration() and drop it in usb_release_interface(), so they should
> already be protected against this particular UAF.

Right.

> The issue concerns a USB device and its parent hub's USB device.  (This makes me
> wonder whether probing a USB device actually requires the parent USB HUB
> device's lock to be held, or whether this is just a consequence of sharing the
> same usb_bus_type?)

Indeed. We could consider making this a device type attribute instead
and move it to usb_if_device_type. May speed up USB enumeration for
people using async probe too.

> In theory a USB device could also keep a reference count to its parent, but then
> the driver core code would rely on an implementation detail of its users; I'd
> prefer not to make driver core's access to dev->parent depend on references
> maintained by individual subsystems.
> 
> I'd even go a step further any say that if subsystems try to keep this pointer
> valid beyond what's guaranteed by the driver core, they should do this with a
> separate bus specific pointer.  The reason is that otherwise the lifetime and
> ownership definition of this pointer does not depend on API contracts, but on
> individual implementation details, which is a very common source for all kinds
> of bugs.

But it's driver core that needs the extra reference. It seems
reasonable to require that any subsystem that sets a new
type->need_parent_lock also makes sure that any device of that type
holds a reference to its parent. And importantly, it's already the case
for the only user of this feature today (i.e. USB interfaces).

Johan