[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

[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

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

[Danilo Krummrich]

On Wed Oct 14, 2026 at 5:14 PM CEST, Johan Hovold wrote:
> 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).

I thought some more about this, and since it is only the USB device type using
it it's probably fine for now.  Should we ever gain new users of this API (which
I do not hope), we can still change it and add something like the diff below.

I'd suggest to also add a comment in the USB code, explaining that this
reference count (also) serves the purpose to fulfill the driver core API
contract.

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 3c36a2128f88..8193bf924726 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -101,6 +101,8 @@ struct driver_private {
  * @async_driver: pointer to device driver awaiting probe via async_probe
  * @deferred_probe_reason: capture the -EPROBE_DEFER message emitted with
  *			   dev_err_probe() for later retrieval via debugfs
+ * @deferred_parent_ref: parent reference held until release for device types
+ *			 with &struct device_type.need_parent_lock set.
  * @device: pointer back to the struct device that this structure is
  *	    associated with.
  * @dead: This device is currently either in the process of or has been
@@ -118,6 +120,7 @@ struct device_private {
 	struct list_head deferred_probe;
 	const struct device_driver *async_driver;
 	char *deferred_probe_reason;
+	struct device *deferred_parent_ref;
 	struct device *device;
 	u8 dead:1;
 };
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5daa88fa724a..45050f90294b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2641,6 +2641,8 @@ static void device_release(struct kobject *kobj)
 	else
 		WARN(1, KERN_ERR "Device '%s' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",
 			dev_name(dev));
+	if (p)
+		put_device(p->deferred_parent_ref);
 	kfree(p);
 }
 
@@ -3682,6 +3684,9 @@ int device_add(struct device *dev)
 	pr_debug("device: '%s': %s\n", dev_name(dev), __func__);
 
 	parent = get_device(dev->parent);
+	/* Parent locking must remain safe after device_del(). */
+	if (dev->type && dev->type->need_parent_lock)
+		dev->p->deferred_parent_ref = get_device(parent);
 	kobj = get_device_parent(dev, parent);
 	if (IS_ERR(kobj)) {
 		error = PTR_ERR(kobj);
@@ -3828,6 +3833,7 @@ int device_add(struct device *dev)
 parent_error:
 	put_device(parent);
 name_error:
+	put_device(dev->p->deferred_parent_ref);
 	kfree(dev->p);
 	dev->p = NULL;
 	goto done;
@@ -4697,6 +4703,10 @@ int device_move(struct device *dev, struct device *new_parent,
 		goto out;
 	}
 	old_parent = dev->parent;
+	if (dev->type && dev->type->need_parent_lock) {
+		dev->p->deferred_parent_ref = get_device(new_parent);
+		put_device(old_parent);
+	}
 	dev->parent = new_parent;
 	if (old_parent)
 		klist_remove(&dev->p->knode_parent);
@@ -4715,6 +4725,10 @@ int device_move(struct device *dev, struct device *new_parent,
 				if (new_parent)
 					klist_remove(&dev->p->knode_parent);
 				dev->parent = old_parent;
+				if (dev->type && dev->type->need_parent_lock) {
+					dev->p->deferred_parent_ref = get_device(old_parent);
+					put_device(new_parent);
+				}
 				if (old_parent) {
 					klist_add_tail(&dev->p->knode_parent,
 						       &old_parent->p->klist_children);
diff --git a/include/linux/device.h b/include/linux/device.h
index 7c7682e4dafe..7604060ff99b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -85,8 +85,7 @@ int subsys_virtual_register(const struct bus_type *subsys,
  * the DEVTYPE variable.
  *
  * @need_parent_lock:	When probing or removing a device of this type,
- *			driver core should lock the device's parent. Note that
- *			the device must hold a reference to its parent.
+ *			driver core should lock the device's parent.
  */
 struct device_type {
 	const char *name;

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

[Danilo Krummrich]

On Thu Oct 15, 2026 at 12:40 AM CEST, Danilo Krummrich wrote:
> On Wed Oct 14, 2026 at 5:14 PM CEST, Johan Hovold wrote:
>> 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).
>
> I thought some more about this, and since it is only the USB device type using
> it it's probably fine for now.  Should we ever gain new users of this API (which
> I do not hope), we can still change it and add something like the diff below.
>
> I'd suggest to also add a comment in the USB code, explaining that this
> reference count (also) serves the purpose to fulfill the driver core API
> contract.

I pasted the wrong diff, I meant the one below, but either should work.

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 3c36a2128f88..b3730d030f5f 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -161,8 +161,9 @@ bool bus_is_registered(const struct bus_type *bus);

 int bus_add_driver(struct device_driver *drv);
 void bus_remove_driver(struct device_driver *drv);
+struct device *device_get_parent_for_lock(struct device *dev);
 void device_release_driver_internal(struct device *dev, const struct device_driver *drv,
-				    struct device *parent);
+				    bool lock_parent);

 void driver_detach(const struct device_driver *drv);
 void driver_deferred_probe_del(struct device *dev);
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index fd2257f73253..9975f250d22e 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -637,11 +637,13 @@ void bus_probe_device(struct device *dev)

 void bus_probe_device_deferred(struct device *dev)
 {
-	if (dev->type && dev->type->need_parent_lock)
-		device_lock(dev->parent);
+	struct device *parent __free(put_device) = device_get_parent_for_lock(dev);
+
+	if (parent)
+		device_lock(parent);
 	__bus_probe_device(dev);
-	if (dev->type && dev->type->need_parent_lock)
-		device_unlock(dev->parent);
+	if (parent)
+		device_unlock(parent);
 }

 /**
@@ -866,11 +868,13 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
 	int ret = 0;

 	if (!dev->driver) {
-		if (dev->type && dev->type->need_parent_lock)
-			device_lock(dev->parent);
+		struct device *parent __free(put_device) = device_get_parent_for_lock(dev);
+
+		if (parent)
+			device_lock(parent);
 		ret = device_attach(dev);
-		if (dev->type && dev->type->need_parent_lock)
-			device_unlock(dev->parent);
+		if (parent)
+			device_unlock(parent);
 	}
 	return ret < 0 ? ret : 0;
 }
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5daa88fa724a..3a924c4e194e 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1676,8 +1676,7 @@ void device_links_unbind_consumers(struct device *dev)

 			device_links_write_unlock();

-			device_release_driver_internal(consumer, NULL,
-						       consumer->parent);
+			device_release_driver_internal(consumer, NULL, true);
 			put_device(consumer);
 			goto start;
 		}
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 52123251965d..6ff8d0653769 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1030,6 +1030,30 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
 	return ret == 0;
 }

+/*
+ * Pin the parent before taking its lock. A reference to dev alone does not
+ * keep its parent alive after device_del().
+ *
+ * device_del() marks the device dead under its lock before dropping the
+ * registration reference to the parent. A dead device may still need to be
+ * unbound; while dev->driver is set, device_del() cannot have completed the
+ * unbind and dropped that reference either.
+ *
+ * The caller must hold a reference to dev and put the returned parent after
+ * its last access, including any temporary unlock/relock during unbind.
+ */
+struct device *device_get_parent_for_lock(struct device *dev)
+{
+	if (!dev->type || !dev->type->need_parent_lock)
+		return NULL;
+
+	guard(device)(dev);
+	if (dev->p->dead && !dev->driver)
+		return NULL;
+
+	return get_device(dev->parent);
+}
+
 /*
  * __device_driver_lock - acquire locks needed to manipulate dev->drv
  * @dev: Device we will update driver info for
@@ -1065,13 +1089,14 @@ static void __device_driver_unlock(struct device *dev, struct device *parent)
 static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
 {
 	struct device *dev = _dev;
+	struct device *parent __free(put_device) = device_get_parent_for_lock(dev);
 	struct device_attach_data data = {
 		.dev		= dev,
 		.check_async	= true,
 		.want_async	= true,
 	};

-	__device_driver_lock(dev, dev->parent);
+	__device_driver_lock(dev, parent);

 	/*
 	 * Check if device has already been removed or claimed. This may
@@ -1093,7 +1118,7 @@ static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
 	if (dev->parent)
 		pm_runtime_put(dev->parent);
 out_unlock:
-	__device_driver_unlock(dev, dev->parent);
+	__device_driver_unlock(dev, parent);

 	put_device(dev);
 }
@@ -1205,11 +1230,12 @@ void device_initial_probe(struct device *dev)
  */
 int device_driver_attach(const struct device_driver *drv, struct device *dev)
 {
+	struct device *parent __free(put_device) = device_get_parent_for_lock(dev);
 	int ret;

-	__device_driver_lock(dev, dev->parent);
+	__device_driver_lock(dev, parent);
 	ret = __driver_probe_device(drv, dev);
-	__device_driver_unlock(dev, dev->parent);
+	__device_driver_unlock(dev, parent);

 	/* also return probe errors as normal negative errnos */
 	if (ret > 0)
@@ -1223,14 +1249,15 @@ EXPORT_SYMBOL_GPL(device_driver_attach);
 static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie)
 {
 	struct device *dev = _dev;
+	struct device *parent __free(put_device) = device_get_parent_for_lock(dev);
 	const struct device_driver *drv;
 	int ret;

-	__device_driver_lock(dev, dev->parent);
+	__device_driver_lock(dev, parent);
 	drv = dev->p->async_driver;
 	dev->p->async_driver = NULL;
 	ret = driver_probe_device(drv, dev);
-	__device_driver_unlock(dev, dev->parent);
+	__device_driver_unlock(dev, parent);

 	dev_dbg(dev, "driver %s async attach completed: %d\n", drv->name, ret);

@@ -1296,9 +1323,11 @@ static int __driver_attach(struct device *dev, void *data)
 		return 0;
 	}

-	__device_driver_lock(dev, dev->parent);
+	struct device *parent __free(put_device) = device_get_parent_for_lock(dev);
+
+	__device_driver_lock(dev, parent);
 	driver_probe_device(drv, dev);
-	__device_driver_unlock(dev, dev->parent);
+	__device_driver_unlock(dev, parent);

 	return 0;
 }
@@ -1372,8 +1401,11 @@ static void __device_release_driver(struct device *dev, struct device *parent)

 void device_release_driver_internal(struct device *dev,
 				    const struct device_driver *drv,
-				    struct device *parent)
+				    bool lock_parent)
 {
+	struct device *parent __free(put_device) = lock_parent ?
+		device_get_parent_for_lock(dev) : NULL;
+
 	__device_driver_lock(dev, parent);

 	if (!drv || drv == dev->driver)
@@ -1400,7 +1432,7 @@ void device_release_driver(struct device *dev)
 	 * within their ->remove callback for the same device, they
 	 * will deadlock right here.
 	 */
-	device_release_driver_internal(dev, NULL, NULL);
+	device_release_driver_internal(dev, NULL, false);
 }
 EXPORT_SYMBOL_GPL(device_release_driver);

@@ -1413,7 +1445,7 @@ EXPORT_SYMBOL_GPL(device_release_driver);
  */
 void device_driver_detach(struct device *dev)
 {
-	device_release_driver_internal(dev, NULL, dev->parent);
+	device_release_driver_internal(dev, NULL, true);
 }

 /**
@@ -1440,7 +1472,7 @@ void driver_detach(const struct device_driver *drv)
 		dev = dev_prv->device;
 		get_device(dev);
 		spin_unlock(&drv->p->klist_devices.k_lock);
-		device_release_driver_internal(dev, drv, dev->parent);
+		device_release_driver_internal(dev, drv, true);
 		put_device(dev);
 	}
 }
diff --git a/include/linux/device.h b/include/linux/device.h
index 7c7682e4dafe..7604060ff99b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -85,8 +85,7 @@ int subsys_virtual_register(const struct bus_type *subsys,
  * the DEVTYPE variable.
  *
  * @need_parent_lock:	When probing or removing a device of this type,
- *			driver core should lock the device's parent. Note that
- *			the device must hold a reference to its parent.
+ *			driver core should lock the device's parent.
  */
 struct device_type {
 	const char *name;