driver core: reject devices with unregistered buses
Subsystems (2)
Johan Hovold
maintainerPATCH 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.
--- drivers/base/bus.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/base/bus.c +9 -2
@@ -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
Danilo Krummrich
maintainerQuoted 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")Cc [email protected] # 6.3
Hm...this sounds like hardening and not like a "real" bug fix. Do you have a specific reason why you added Cc: stable?
Johan Hovold
maintainerQuoted 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")Cc [email protected] # 6.3Hm...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
Danilo Krummrich
maintainerQuoted 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")Cc [email protected] # 6.3Hm...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.
Johan Hovold
maintainerQuoted 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")Cc [email protected] # 6.3Hm...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
Danilo Krummrich
maintainerQuoted 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.
Johan Hovold
maintainerQuoted 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
Danilo Krummrich
maintainerQuoted 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.)
Johan Hovold
maintainerQuoted 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
Danilo Krummrich
maintainerQuoted 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