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

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

[Johan Hovold]

USB interfaces should be probed with the parent USB device lock held,
but this is not honoured when probe is deferred.

Add a new bus_probe_device_deferred() helper to handle deferred probe
where the parent device lock is not held.

Fixes: d1c3414c2a9d ("drivercore: Add driver probe deferral mechanism")
Reported-by: Mario Peter <mario.peter@leica-geosystems.com>
Link: https://lore.kernel.org/20261007131514.1260696-1-mario.peter@leica-geosystems.com
Cc: stable@vger.kernel.org	# 3.4
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/base/base.h |  1 +
 drivers/base/bus.c  | 28 +++++++++++++++++++++-------
 drivers/base/dd.c   |  7 ++++---
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index a5b7abc10ff0..3c36a2128f88 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -154,6 +154,7 @@ struct kobject *virtual_device_parent(void);
 
 int bus_add_device(struct device *dev);
 void bus_probe_device(struct device *dev);
+void bus_probe_device_deferred(struct device *dev);
 void bus_remove_device(struct device *dev);
 void bus_notify(struct device *dev, enum bus_notifier_event value);
 bool bus_is_registered(const struct bus_type *bus);
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index c51ad96d4de4..fd62594eb6f9 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -606,13 +606,7 @@ int bus_add_device(struct device *dev)
 	return error;
 }
 
-/**
- * bus_probe_device - probe drivers for a new device
- * @dev: device to probe
- *
- * - Automatically probe for a driver if the bus allows it.
- */
-void bus_probe_device(struct device *dev)
+static void __bus_probe_device(struct device *dev)
 {
 	struct subsys_private *sp = bus_to_subsys(dev->bus);
 	struct subsys_interface *sif;
@@ -630,6 +624,26 @@ void bus_probe_device(struct device *dev)
 	subsys_put(sp);
 }
 
+/**
+ * bus_probe_device - probe drivers for a new device
+ * @dev: device to probe
+ *
+ * - Automatically probe for a driver if the bus allows it.
+ */
+void bus_probe_device(struct device *dev)
+{
+	__bus_probe_device(dev);
+}
+
+void bus_probe_device_deferred(struct device *dev)
+{
+	if (dev->parent && dev->bus->need_parent_lock)
+		device_lock(dev->parent);
+	__bus_probe_device(dev);
+	if (dev->parent && dev->bus->need_parent_lock)
+		device_unlock(dev->parent);
+}
+
 /**
  * bus_remove_device - remove device from bus
  * @dev: device to be removed
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9dc7cce9bd8d..87962f4e914c 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -86,8 +86,9 @@ static void deferred_probe_work_func(struct work_struct *work)
 	/*
 	 * This block processes every device in the deferred 'active' list.
 	 * Each device is removed from the active list and passed to
-	 * bus_probe_device() to re-attempt the probe.  The loop continues
-	 * until every device in the active list is removed and retried.
+	 * bus_probe_device_deferred() to re-attempt the probe.  The loop
+	 * continues until every device in the active list is removed and
+	 * retried.
 	 *
 	 * Note: Once the device is removed from the list and the mutex is
 	 * released, it is possible for the device get freed by another thread
@@ -121,7 +122,7 @@ static void deferred_probe_work_func(struct work_struct *work)
 		device_pm_move_to_tail(dev);
 
 		dev_dbg(dev, "Retrying from deferred list\n");
-		bus_probe_device(dev);
+		bus_probe_device_deferred(dev);
 		mutex_lock(&deferred_probe_mutex);
 
 		put_device(dev);
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] driver core: unbalanced parent lock in deferred probe due to TOCTOU on dev->parent
- [High] driver core: Use-After-Free of dev->parent in deferred probe
--

commit fa3fc53326d3b088884e2e7e5dc50b5162e40379
Author: Johan Hovold <johan@kernel.org>

driver core: take parent lock on deferred probe

This commit adds a new helper bus_probe_device_deferred() to take
the parent device lock during deferred probe. It modifies the deferred
probe worker to use this helper instead of bus_probe_device().

Link: https://lore.kernel.org/20261007131514.1260696-1-mario.peter@leica-geosystems.com

> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 18f12c6f910a0..ebd069d98d255 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
[ ... ]
> @@ -630,6 +624,26 @@ void bus_probe_device(struct device *dev)
[ ... ]
> +void bus_probe_device_deferred(struct device *dev)
> +{
> +	if (dev->parent && dev->bus->need_parent_lock)
> +		device_lock(dev->parent);

[Severity: High]
Does this code risk a use-after-free of the parent device?

When called from deferred_probe_work_func(), the worker thread has only
taken a reference via get_device(dev) and dropped the deferred_probe_mutex.

If device_del(dev) runs concurrently, it can drop the device's reference to
its parent via put_device(parent). If that was the last reference, the parent
object could be freed immediately.

Because we only pinned the device itself and not the parent, could this read
of dev->parent and the subsequent device_lock() access freed memory?

> +	__bus_probe_device(dev);
> +	if (dev->parent && dev->bus->need_parent_lock)
> +		device_unlock(dev->parent);

[Severity: High]
Is it possible for dev->parent to change during the execution of
__bus_probe_device(dev)?

If a concurrent thread executes device_move() on the device while we are
probing, dev->parent could be updated. This happens because device_move()
updates dev->parent while only holding device_pm_lock(), which is not held
here.

This would result in evaluating dev->parent a second time and potentially
calling device_unlock() on the new parent instead of the one that was
originally locked, causing a lock imbalance.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013140615.1226992-1-johan@kernel.org?part=1

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

[Danilo Krummrich]

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?

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);
> +}

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

[Alan Stern]

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?

Alan Stern

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

[Danilo Krummrich]

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().

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.

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?)

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.

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

[Alan Stern]

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().

There could be another reference that extends all the way to the child's 
release.

> 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.
> 
> 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?)

The latter.

> 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.

I was going to suggest that the driver core could acquire and release 
these extra references automatically for devices on a bus with 
need_parent_lock set, but it looks like that can't be done because the 
bus_type might not yet be set when device_initialize() runs.

However, is there any real reason why the driver core wouldn't want to 
guarantee that dev->parent will be a valid pointer up until the time 
when dev is released?  For every device that has a parent, not just USB?

On the other hand, there's something a bit weird about this whole thing.  
The problem arises when you want to probe a device whose parent has been 
deleted.  Isn't there some sort of protection to prevent that from ever 
happening?  I mean, doesn't deleting a device imply that all its 
descendants have been (or are in the process of being) deleted?  And of 
course, deleting and probing are mutually exclusive -- they can't race, 
right?

Alan Stern

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

[Danilo Krummrich]

On Wed Oct 14, 2026 at 4:38 AM CEST, Alan Stern wrote:
> I was going to suggest that the driver core could acquire and release 
> these extra references automatically for devices on a bus with 
> need_parent_lock set, but it looks like that can't be done because the 
> bus_type might not yet be set when device_initialize() runs.
>
> However, is there any real reason why the driver core wouldn't want to 
> guarantee that dev->parent will be a valid pointer up until the time 
> when dev is released?  For every device that has a parent, not just USB?

I'm not sure we don't have any cases where this would create cyclic reference
counts.  Like with the kobject API, the separate device_del() step would break
circular reference counts.

From a quick check ALSA seems to have this cyclic topology.

> On the other hand, there's something a bit weird about this whole thing.  
> The problem arises when you want to probe a device whose parent has been 
> deleted.  Isn't there some sort of protection to prevent that from ever 
> happening?  I mean, doesn't deleting a device imply that all its 
> descendants have been (or are in the process of being) deleted?  And of 
> course, deleting and probing are mutually exclusive -- they can't race, 
> right?

Yes, that's what the dev->p->dead check under the device lock protects against.
But the new code accesses dev->parent before acquiring the device lock and
reaching that check.

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

[Johan Hovold]

On Wed, Oct 14, 2026 at 04:30:48PM +0200, Danilo Krummrich wrote:
> On Wed Oct 14, 2026 at 4:38 AM CEST, Alan Stern wrote:

> > On the other hand, there's something a bit weird about this whole thing.  
> > The problem arises when you want to probe a device whose parent has been 
> > deleted.  Isn't there some sort of protection to prevent that from ever 
> > happening?  I mean, doesn't deleting a device imply that all its 
> > descendants have been (or are in the process of being) deleted?  And of 
> > course, deleting and probing are mutually exclusive -- they can't race, 
> > right?
> 
> Yes, that's what the dev->p->dead check under the device lock protects against.
> But the new code accesses dev->parent before acquiring the device lock and
> reaching that check.

It's not just the new code; we have the same issue in __driver_attach()
bus_rescan_devices_helper() for synchronous probe too.

Johan

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

[Johan Hovold]

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

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

[Danilo Krummrich]

On Wed Oct 14, 2026 at 10:15 AM CEST, Johan Hovold wrote:
> On Tue, Oct 13, 2026 at 11:01:16PM +0200, Danilo Krummrich wrote:
>> 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.

Great, that sounds like the better approach to me then.

>> 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).

Exactly, it is the driver core that needs the extra reference count; for a field
that is owned by the driver core, and for which the driver core manages its
lifetime.  If it needs an additional reference count for async or deferred work
it should obtain the reference count for this scope explicitly.

USB interfaces already holding an additional reference makes the proposed
contract work for today's user, but I'd rather not make that prerequisite of the
API.  It would push a prerequisite for an implementation detail to the user.

Please, let's not add this API contract. Let's have the driver core deal with
this internally.

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

[Johan Hovold]

On Wed, Oct 14, 2026 at 04:42:09PM +0200, Danilo Krummrich wrote:
> On Wed Oct 14, 2026 at 10:15 AM CEST, Johan Hovold wrote:

> > 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).
> 
> Exactly, it is the driver core that needs the extra reference count; for a field
> that is owned by the driver core, and for which the driver core manages its
> lifetime.  If it needs an additional reference count for async or deferred work
> it should obtain the reference count for this scope explicitly.

As I've mentioned, it's not just for async or deferred probe. It's also
needed when a driver gets added or when probe is triggered through
sysfs. The bus iterators only takes a reference to the each device and
not their parents before calling __driver_attach() etc.

> USB interfaces already holding an additional reference makes the proposed
> contract work for today's user, but I'd rather not make that prerequisite of the
> API.  It would push a prerequisite for an implementation detail to the user.

I understand your concern, but my reasoning is that this feature is only
needed when devices are tightly coupled with their parents and where it
therefore probably always make sense to pin the parent.

Note that the need_parent_lock flag is now set in the device type struct
which also holds a pointer to the release function that drops the
reference. So it's pretty easy to check any theoretical further users.

> Please, let's not add this API contract. Let's have the driver core deal with
> this internally.

I can take another look at it, but the alternatives seemed either error
prone or a bit icky (e.g. a helper that returns a reference to the
parent after checking the dead flag under the device lock).

Johan