Skip to contentSkip to conversation

driver core: reject devices with unregistered buses

PATCH Superseded Johan Hovold 2 participants 10 messages Apr 27 – Apr 29

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Johan Hovold

author maintainer
#0 lore raw

PATCH 1/1 driver core: reject devices with unregistered buses

Trying to register a device on a bus which has not yet been registered used to trigger a NULL-pointer dereference, but since the const bus structure rework registration instead succeeds without the device being added to the bus.

Reject devices with unregistered buses to catch any callers that get the ordering wrong and to handle bus registration failures more gracefully.

Fixes 5221b82d46f2 ("driver core: bus: bus_add/probe/remove_device() cleanups")
Cc Greg Kroah-Hartman <[email protected]>
---
 drivers/base/bus.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/base/bus.c +9 -2
index 8b6722ff8590..d17bd91490ee 100644
@@ -544,10 +544,10 @@ static const struct attribute_group driver_override_dev_group = {
  */
 int bus_add_device(struct device *dev)
 {
-	struct subsys_private *sp = bus_to_subsys(dev->bus);
+	struct subsys_private *sp;
 	int error;
 
-	if (!sp) {
+	if (!dev->bus) {
 		/*
 		 * This is a normal operation for many devices that do not
 		 * have a bus assigned to them, just say that all went
@@ -556,6 +556,13 @@ int bus_add_device(struct device *dev)
 		return 0;
 	}
 
+	sp = bus_to_subsys(dev->bus);
+	if (!sp) {
+		pr_err("%s: cannot add device '%s' to unregistered bus '%s'\n",
+		       __func__, dev_name(dev), dev->bus->name);
+		return -EINVAL;
+	}
+
 	/*
 	 * Reference in sp is now incremented and will be dropped when
 	 * the device is removed from the bus
Signature · 1 line
2.53.0
maintainer
#1 lore raw
Quoted 11 lines · Johan Hovold

Trying to register a device on a bus which has not yet been registered used to trigger a NULL-pointer dereference, but since the const bus structure rework registration instead succeeds without the device being added to the bus.

Reject devices with unregistered buses to catch any callers that get the ordering wrong and to handle bus registration failures more gracefully.

Fixes 5221b82d46f2 ("driver core: bus: bus_add/probe/remove_device() cleanups")

Hm...this sounds like hardening and not like a "real" bug fix. Do you have a specific reason why you added Cc: stable?

author maintainer
#2 lore raw
Quoted 10 lines · Danilo Krummrich
Quoted 9 lines · Johan Hovold

Trying to register a device on a bus which has not yet been registered used to trigger a NULL-pointer dereference, but since the const bus structure rework registration instead succeeds without the device being added to the bus.

Reject devices with unregistered buses to catch any callers that get the ordering wrong and to handle bus registration failures more gracefully.

Quoted 5 lines
Quoted 2 lines
Fixes 5221b82d46f2 ("driver core: bus: bus_add/probe/remove_device() cleanups")

Hm...this sounds like hardening and not like a "real" bug fix. Do you have a specific reason why you added Cc: stable?

It's certainly a bug fix and this change in behaviour was clearly unintended.

Any caller getting the ordering wrong would now succeed in registering devices, but no driver would ever be bound which is harder to detect than the earlier crashes.

Whether any offenders have snuck in since 6.3 I don't know, but I still think this warrants a backport.

Johan

maintainer
#3 lore raw
Quoted 20 lines · Johan Hovold
Quoted 10 lines · Danilo Krummrich
Quoted 9 lines · Johan Hovold

Trying to register a device on a bus which has not yet been registered used to trigger a NULL-pointer dereference, but since the const bus structure rework registration instead succeeds without the device being added to the bus.

Reject devices with unregistered buses to catch any callers that get the ordering wrong and to handle bus registration failures more gracefully.

Quoted 5 lines
Quoted 2 lines
Fixes 5221b82d46f2 ("driver core: bus: bus_add/probe/remove_device() cleanups")

Hm...this sounds like hardening and not like a "real" bug fix. Do you have a specific reason why you added Cc: stable?

It's certainly a bug fix and this change in behaviour was clearly unintended.

Quoted 6 lines

Any caller getting the ordering wrong would now succeed in registering devices, but no driver would ever be bound which is harder to detect than the earlier crashes.

Whether any offenders have snuck in since 6.3 I don't know, but I still think this warrants a backport.

I see where you are coming from, and I agree that having an explicit error print is an improvement over "the device just never got probed".

However, this isn't an actual bug -- it just happens to make a "real" bug less obvious to catch.

That said, I don't see how this warrants a stable backport, i.e. it doesn't even fall under the "this could be a problem" or "theoretical bug" category, which typically are not accepted either.

As mentioned in the other thread, if this was relaxed, I'm happy to hear about it.

author maintainer
#4 lore raw
Quoted 25 lines · Danilo Krummrich
Quoted 24 lines · Johan Hovold
Quoted 10 lines · Danilo Krummrich
Quoted 9 lines · Johan Hovold

Trying to register a device on a bus which has not yet been registered used to trigger a NULL-pointer dereference, but since the const bus structure rework registration instead succeeds without the device being added to the bus.

Reject devices with unregistered buses to catch any callers that get the ordering wrong and to handle bus registration failures more gracefully.

Quoted 5 lines
Quoted 2 lines
Fixes 5221b82d46f2 ("driver core: bus: bus_add/probe/remove_device() cleanups")

Hm...this sounds like hardening and not like a "real" bug fix. Do you have a specific reason why you added Cc: stable?

It's certainly a bug fix and this change in behaviour was clearly unintended.

Any caller getting the ordering wrong would now succeed in registering devices, but no driver would ever be bound which is harder to detect than the earlier crashes.

Quoted 8 lines
Quoted 2 lines

Whether any offenders have snuck in since 6.3 I don't know, but I still think this warrants a backport.

I see where you are coming from, and I agree that having an explicit error print is an improvement over "the device just never got probed".

However, this isn't an actual bug -- it just happens to make a "real" bug less obvious to catch.

It seems we have differing definitions of "bug", but to me this is clearly a bug in driver core. Whether anyone will hit it, is a separate issue.

But if we have any racing subsystem vs device registrations this would allow us to catch and fix those more easily. And the fix is straight forward and only turns a silent breakage into a properly logged error.

Quoted 3 lines

That said, I don't see how this warrants a stable backport, i.e. it doesn't even fall under the "this could be a problem" or "theoretical bug" category, which typically are not accepted either.

Yeah, under the documented rules it may be a bit of a stretch unless you consider it a "oh, that's not good" issue.

Quoted 2 lines

As mentioned in the other thread, if this was relaxed, I'm happy to hear about it.

It has been in practice as I mentioned there.

Johan

maintainer
#5 lore raw
Quoted 2 lines · Johan Hovold

It seems we have differing definitions of "bug", but to me this is clearly a bug in driver core.

I think we do -- you are saying it is a bug that the driver core does not print a warning when a caller introduces a bug on its own due to an ordering violation.

While it clearly is an improvement, I indeed do not consider this a bug.

author maintainer
#6 lore raw
Quoted 7 lines · Danilo Krummrich
Quoted 2 lines · Johan Hovold

It seems we have differing definitions of "bug", but to me this is clearly a bug in driver core.

I think we do -- you are saying it is a bug that the driver core does not print a warning when a caller introduces a bug on its own due to an ordering violation.

No, I'm saying that it's a bug in driver core to silently treat a device that is registered before its bus as a bus-less device.

Johan

maintainer
#7 lore raw
Quoted 2 lines · Johan Hovold

No, I'm saying that it's a bug in driver core to silently treat a device that is registered before its bus as a bus-less device.

This is an argument that I can buy into, but in the previous discussion (and in the commit message) the whole motivation evolved around "reject devices with unregistered buses to catch any callers that get the ordering wrong", i.e. catch other people's bugs.

What you are raising now is "the driver core is conflating no bus with unregistered bus handling". However, the commit message does not reflect that at all.

Can you please adjust the commit message accordingly?

(As for the stable question, I don't think this changes anything though.)

author maintainer
#8 lore raw
Quoted 4 lines · Danilo Krummrich
Quoted 2 lines · Johan Hovold

No, I'm saying that it's a bug in driver core to silently treat a device that is registered before its bus as a bus-less device.

Quoted 8 lines

This is an argument that I can buy into, but in the previous discussion (and in the commit message) the whole motivation evolved around "reject devices with unregistered buses to catch any callers that get the ordering wrong", i.e. catch other people's bugs.

What you are raising now is "the driver core is conflating no bus with unregistered bus handling". However, the commit message does not reflect that at all.

The commit message already explains the issue:

Trying to register a device on a bus which has not yet been registered used to trigger a NULL-pointer dereference, but since the const bus structure rework registration instead succeeds without the device being added to the bus.

namely that

registration [...] succeeds [but] without the device being added to the bus.

Quoted 1 line

Can you please adjust the commit message accordingly?

Perhaps I can add "(i.e. as if it were a bus-less device)" to stress it more but I'm not sure it's needed.

Johan

maintainer
#9 lore raw
Quoted 4 lines · Johan Hovold

namely that

registration [...] succeeds [but] without the device being added to the bus.

[...]

Quoted 2 lines

Perhaps I can add "(i.e. as if it were a bus-less device)" to stress it more but I'm not sure it's needed.

I think that would be good, thanks!

Your above quote only vaguely implies that and I think it is pretty hard to infer that without already being aware of this implication.

It even deserves more emphasis; the current commit message reads as if it's only about adding an additional warning.

Thanks,
Danilo