Skip to contentSkip to conversation

driver core: faux: fix root device registration

PATCH Applied Johan Hovold 3 participants 13 messages Apr 24 – Apr 30

Subsystems (2)
Stable branch Driver core
lore
Share & tools
lore

Johan Hovold

author maintainer
#0 lore raw

v2 cover driver core: faux: fix root device registration

This series fixes the faux bus root device registration by using root_device_register().

Included is also a related cleanup.

Johan

Changes in v2:
- only set the faux_bus_root pointer on successful registration
- clean up the init error handling

Johan Hovold (2):
driver core: faux: fix root device registration
driver core: faux: clean up init error handling

 drivers/base/faux.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)
Signature · 1 line
2.53.0
author maintainer
#1 lore raw

PATCH v2 1/2 driver core: faux: fix root device registration

A recent change made the faux bus root device be allocated dynamically but failed to provide a release function to free the memory when the last reference is dropped (on theoretical failure to register the device or bus).

Fix this by using root_device_register() instead of open coding.

Also add the missing sanity check when registering faux devices to avoid use-after-free if the bus failed to register (which would previously have triggered a bunch of use-after-free warnings).

Fixes 61b76d07d2b4 ("driver core: faux: stop using static struct device")
Cc [email protected] # 7.0
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/base/faux.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)
drivers/base/faux.c +10 -12
index fb3e42f21362..3d1d1eafb473 100644
@@ -133,6 +133,9 @@ struct faux_device *faux_device_create_with_groups(const char *name,
 	struct device *dev;
 	int ret;
 
+	if (!faux_bus_root)
+		return NULL;
+
 	faux_obj = kzalloc_obj(*faux_obj);
 	if (!faux_obj)
 		return NULL;

@@ -232,19 +235,12 @@ EXPORT_SYMBOL_GPL(faux_device_destroy);
 
 int __init faux_bus_init(void)
 {
+	struct device *root;
 	int ret;
 
-	faux_bus_root = kzalloc_obj(*faux_bus_root);
-	if (!faux_bus_root)
-		return -ENOMEM;
-
-	dev_set_name(faux_bus_root, "faux");
-
-	ret = device_register(faux_bus_root);
-	if (ret) {
-		put_device(faux_bus_root);
-		return ret;
-	}
+	root = root_device_register("faux");
+	if (IS_ERR(root))
+		return PTR_ERR(root);
 
 	ret = bus_register(&faux_bus_type);
 	if (ret)

@@ -254,12 +250,14 @@ int __init faux_bus_init(void)
 	if (ret)
 		goto error_driver;
 
+	faux_bus_root = root;
+
 	return ret;
 
 error_driver:
 	bus_unregister(&faux_bus_type);
 
 error_bus:
-	device_unregister(faux_bus_root);
+	root_device_unregister(root);
 	return ret;
 }
Signature · 1 line
2.53.0
maintainer
#2 lore raw
Quoted 5 lines · Johan Hovold

A recent change made the faux bus root device be allocated dynamically but failed to provide a release function to free the memory when the last reference is dropped (on theoretical failure to register the device or bus).

Quoted 8 lines

Fix this by using root_device_register() instead of open coding.

Also add the missing sanity check when registering faux devices to avoid use-after-free if the bus failed to register (which would previously have triggered a bunch of use-after-free warnings).

Fixes 61b76d07d2b4 ("driver core: faux: stop using static struct device")
Cc [email protected] # 7.0

I think this is more of a theoretical issue, do we need this in stable trees?

author maintainer
#3 lore raw
Quoted 8 lines · Danilo Krummrich
Quoted 7 lines · Johan Hovold

A recent change made the faux bus root device be allocated dynamically but failed to provide a release function to free the memory when the last reference is dropped (on theoretical failure to register the device or bus).

Fix this by using root_device_register() instead of open coding.

Quoted 8 lines
Quoted 6 lines

Also add the missing sanity check when registering faux devices to avoid use-after-free if the bus failed to register (which would previously have triggered a bunch of use-after-free warnings).

Fixes 61b76d07d2b4 ("driver core: faux: stop using static struct device")
Cc [email protected] # 7.0

I think this is more of a theoretical issue, do we need this in stable trees?

Sure, this is borderline, but given that autosel would probably pick it up anyway we might as well mark it directly.

Johan

maintainer
#4 lore raw
Quoted 13 lines · Johan Hovold
Quoted 12 lines · Danilo Krummrich
Quoted 11 lines · Johan Hovold

A recent change made the faux bus root device be allocated dynamically but failed to provide a release function to free the memory when the last reference is dropped (on theoretical failure to register the device or bus).

Fix this by using root_device_register() instead of open coding.

Also add the missing sanity check when registering faux devices to avoid use-after-free if the bus failed to register (which would previously have triggered a bunch of use-after-free warnings).

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Fixes 61b76d07d2b4 ("driver core: faux: stop using static struct device")
Cc [email protected] # 7.0

I think this is more of a theoretical issue, do we need this in stable trees?

Sure, this is borderline, but given that autosel would probably pick it up anyway we might as well mark it directly.

In such a case developers should probably give the stable team a hint that the patch in question does not need backporting.

But using the expectation that autosel may pick it up as a justification to mark it for stable in the first place seems wrong.

The stable documentation [1] is very clear that theoretical issues must not be backported into stable trees unless an explanation of how the bug can be exploited can be provided.

If that was relaxed in some way, it probably needs updating.

[1] https://docs.kernel.org/process/stable-kernel-rules.html#everything-you-ever-wanted-to-know-about-linux-stable-releases

author maintainer
#5 lore raw
Quoted 19 lines · Danilo Krummrich
Quoted 18 lines · Johan Hovold
Quoted 16 lines · Danilo Krummrich
Quoted 5 lines · Johan Hovold

A recent change made the faux bus root device be allocated dynamically but failed to provide a release function to free the memory when the last reference is dropped (on theoretical failure to register the device or bus).

Quoted 8 lines

Fix this by using root_device_register() instead of open coding.

Also add the missing sanity check when registering faux devices to avoid use-after-free if the bus failed to register (which would previously have triggered a bunch of use-after-free warnings).

Fixes 61b76d07d2b4 ("driver core: faux: stop using static struct device")
Cc [email protected] # 7.0

I think this is more of a theoretical issue, do we need this in stable trees?

Quoted 8 lines
Quoted 2 lines

Sure, this is borderline, but given that autosel would probably pick it up anyway we might as well mark it directly.

In such a case developers should probably give the stable team a hint that the patch in question does not need backporting.

But using the expectation that autosel may pick it up as a justification to mark it for stable in the first place seems wrong.

It fixes a memory leak (and some warnings). We backport such fixes all the time, including in other error paths which are unlikely to ever be hit.

Quoted 5 lines

The stable documentation [1] is very clear that theoretical issues must not be backported into stable trees unless an explanation of how the bug can be exploited can be provided.

If that was relaxed in some way, it probably needs updating.

The stable rules have been relaxed in practice since Autosel. Anything that looks like a fix (e.g. has a Fixes tag) gets backported.

And people get tired of asking the stable team to drop patches that were not marked for backporting.

But feel free to drop the CC-stable tag here if you want to.

Johan

maintainer
#6 lore raw
Quoted 5 lines · Johan Hovold

The stable rules have been relaxed in practice since Autosel. Anything that looks like a fix (e.g. has a Fixes tag) gets backported.

And people get tired of asking the stable team to drop patches that were not marked for backporting.

There is still a difference between this happening in practice and using that as a justification to explicitly request a stable backport for something that - by the official rules - isn't mandated for stable backport.

If the rules changed to the point that they don't apply anymore for deciding whether to stick Cc: stable on a patch or not, then they should be adjusted accordingly.

author maintainer
#7 lore raw
Quoted 10 lines · Danilo Krummrich
Quoted 5 lines · Johan Hovold

The stable rules have been relaxed in practice since Autosel. Anything that looks like a fix (e.g. has a Fixes tag) gets backported.

And people get tired of asking the stable team to drop patches that were not marked for backporting.

There is still a difference between this happening in practice and using that as a justification to explicitly request a stable backport for something that - by the official rules - isn't mandated for stable backport.

Again, feel free to drop the CC stable tag if you want to.

Johan

maintainer
#8 lore raw
Quoted 1 line · Johan Hovold

Again, feel free to drop the CC stable tag if you want to.

It's not so much about what I want -- I just try to stay close to what the guideline is, and only deviate when there's a good reason.

maintainer
#9 lore raw
Quoted 6 lines · Danilo Krummrich
Quoted 1 line · Johan Hovold

Again, feel free to drop the CC stable tag if you want to.

It's not so much about what I want -- I just try to stay close to what the guideline is, and only deviate when there's a good reason.

Personally, I think it's good to backport this. I trust Johan here so I have no objection to taking this for stable trees (nor the other patches he tagged for stable).

The rules are there for the stable maintainers to say "no" to, not so much for us to be extremely strict about it at times if it makes sense to us.

thanks,

greg k-h

maintainer
#10 lore raw
Quoted 6 lines · Greg Kroah-Hartman
Quoted 3 lines · Danilo Krummrich

It's not so much about what I want -- I just try to stay close to what the guideline is, and only deviate when there's a good reason.

Personally, I think it's good to backport this.

That's good to know, I personally also lean more towards backporting such things. But for the above reason, I did abstain from it so far, unless I could really make a strong case.

Quoted 3 lines

The rules are there for the stable maintainers to say "no" to, not so much for us to be extremely strict about it at times if it makes sense to us.

Thanks for clarifying!

Generally speaking, I don't think this is about being extremely strict about it; the stable documentation itself is rather strictly worded I think.

I wonder if we can improve the wording without giving up on the "stable maintainers reserve their right to say no" part to better reflect this.

author maintainer
#11 lore raw

PATCH v2 2/2 driver core: faux: clean up init error handling

In reply to Johan Hovold

Clean up the faux bus init error handling by naming the labels after what they do (rather than from where they are jumped to) and separating the success path more clearly by returning explicit zero.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/base/faux.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/base/faux.c +6 -6
index 3d1d1eafb473..a8329f88222e 100644
@@ -244,20 +244,20 @@ int __init faux_bus_init(void)
 
 	ret = bus_register(&faux_bus_type);
 	if (ret)
-		goto error_bus;
+		goto err_deregister_root;
 
 	ret = driver_register(&faux_driver);
 	if (ret)
-		goto error_driver;
+		goto err_deregister_bus;
 
 	faux_bus_root = root;
 
-	return ret;
+	return 0;
 
-error_driver:
+err_deregister_bus:
 	bus_unregister(&faux_bus_type);
-
-error_bus:
+err_deregister_root:
 	root_device_unregister(root);
+
 	return ret;
 }
Signature · 1 line
2.53.0
maintainer Applied
#12 lore raw
In reply to Johan Hovold
Quoted 1 line · Johan Hovold

[PATCH v2 0/2] driver core: faux: fix root device registration

Applied, thanks!

Branch: driver-core-testing
Tree: git://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git

[1/2] driver core: faux: fix root device registration
commit: a1ebb5fd7cc9
[2/2] driver core: faux: clean up init error handling

      commit: a54d0e24fdda

The patches will appear in the next linux-next integration (typically within 24 hours on weekdays).

The patches are in the driver-core-testing branch and will be promoted to driver-core-next after validation.