driver core: take parent lock on deferred probe

drivers/base/bus.c +4 -0

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

drivers/base/bus.c void bus_probe_device(struct device *dev) · line 641

Quoted 4 lines · Johan Hovold

drivers/base/bus.c +4 -0

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

drivers/base/bus.c void bus_probe_device(struct device *dev) · line 645

Quoted 4 lines

drivers/base/bus.c +4 -0

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

Quoted 11 lines · Danilo Krummrich

drivers/base/bus.c void bus_probe_device(struct device *dev) · line 641

Quoted 4 lines · Johan Hovold

drivers/base/bus.c +4 -0

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

Quoted 14 lines

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.

Quoted 4 lines

drivers/base/bus.c void bus_probe_device(struct device *dev) · line 645

Quoted 4 lines

drivers/base/bus.c +4 -0

+	__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?

Quoted 29 lines · Alan Stern

Quoted 11 lines · Danilo Krummrich

drivers/base/bus.c void bus_probe_device(struct device *dev) · line 641

Quoted 4 lines · Johan Hovold

drivers/base/bus.c +4 -0

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

Quoted 14 lines

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.

Quoted 7 lines

Quoted 4 lines

drivers/base/bus.c void bus_probe_device(struct device *dev) · line 645

Quoted 4 lines

drivers/base/bus.c +4 -0

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

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?

Quoted 7 lines · Alan Stern

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.

Quoted 29 lines · Alan Stern

Quoted 11 lines · Danilo Krummrich

drivers/base/bus.c void bus_probe_device(struct device *dev) · line 641

Quoted 4 lines · Johan Hovold

drivers/base/bus.c +4 -0

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

Quoted 14 lines

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.

Quoted 7 lines

Quoted 4 lines

drivers/base/bus.c void bus_probe_device(struct device *dev) · line 645

Quoted 4 lines

drivers/base/bus.c +4 -0

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

Quoted 4 lines · Danilo Krummrich

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.

Quoted 11 lines

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

Quoted 5 lines · Johan Hovold

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.

Quoted 8 lines · Danilo Krummrich

Quoted 5 lines · Johan Hovold

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

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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

Quoted 31 lines · Johan Hovold

Quoted 8 lines · Danilo Krummrich

Quoted 5 lines · Johan Hovold

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

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 13 lines · Johan Hovold

On Wed, Oct 14, 2026 at 04:42:09PM +0200, Danilo Krummrich wrote:

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.

Quoted 2 lines

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

Quoted 3 lines

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.