Hi,
These two patches fix some minor error path mistakes in the device module.
Changes -------
v1->v2 * Add fixes tag * Add imperative statement in the commit description v2->v3 * Add a goto exit label kunit_device_register_internal
Wander Lairson Costa (2): kunit: unregister the device on error kunit: avoid memory leak on device register error
lib/kunit/device.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
kunit_init_device() should unregister the device on bus register error, but mistakenly it tries to unregister the bus.
Unregister the device instead of the bus.
Signed-off-by: Wander Lairson Costa wander@redhat.com Fixes: d03c720e03bd ("kunit: Add APIs for managing devices") --- lib/kunit/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/kunit/device.c b/lib/kunit/device.c index abc603730b8e..25c81ed465fb 100644 --- a/lib/kunit/device.c +++ b/lib/kunit/device.c @@ -51,7 +51,7 @@ int kunit_bus_init(void)
error = bus_register(&kunit_bus_type); if (error) - bus_unregister(&kunit_bus_type); + root_device_unregister(kunit_bus_device); return error; }
On Fri, 19 Apr 2024 at 05:02, Wander Lairson Costa wander@redhat.com wrote:
kunit_init_device() should unregister the device on bus register error, but mistakenly it tries to unregister the bus.
Unregister the device instead of the bus.
Signed-off-by: Wander Lairson Costa wander@redhat.com Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
Nice catch!
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
lib/kunit/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/kunit/device.c b/lib/kunit/device.c index abc603730b8e..25c81ed465fb 100644 --- a/lib/kunit/device.c +++ b/lib/kunit/device.c @@ -51,7 +51,7 @@ int kunit_bus_init(void)
error = bus_register(&kunit_bus_type); if (error)
bus_unregister(&kunit_bus_type);
root_device_unregister(kunit_bus_device); return error;}
-- 2.44.0
kunit_init_device() should unregister the device on bus register error, but mistakenly it tries to unregister the bus.
Would the following description variant be more appropriate?
kunit_init_device() should unregister the device on bus registration error. But it mistakenly tries to unregister the bus.
Regards, Markus
On Fri, Apr 19, 2024 at 07:40:43AM +0200, Markus Elfring wrote:
kunit_init_device() should unregister the device on bus register error, but mistakenly it tries to unregister the bus.
Would the following description variant be more appropriate?
kunit_init_device() should unregister the device on bus registration error. But it mistakenly tries to unregister the bus.
Hi,
This is the semi-friendly patch-bot of Greg Kroah-Hartman.
Markus, you seem to have sent a nonsensical or otherwise pointless review comment to a patch submission on a Linux kernel developer mailing list. I strongly suggest that you not do this anymore. Please do not bother developers who are actively working to produce patches and features with comments that, in the end, are a waste of time.
Patch submitter, please ignore Markus's suggestion; you do not need to follow it at all. The person/bot/AI that sent it is being ignored by almost all Linux kernel maintainers for having a persistent pattern of behavior of producing distracting and pointless commentary, and inability to adapt to feedback. Please feel free to also ignore emails from them.
thanks,
greg k-h's patch email bot
If the device register fails, free the allocated memory before returning.
Signed-off-by: Wander Lairson Costa wander@redhat.com Fixes: d03c720e03bd ("kunit: Add APIs for managing devices") --- lib/kunit/device.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/lib/kunit/device.c b/lib/kunit/device.c index 25c81ed465fb..bc2e2032e505 100644 --- a/lib/kunit/device.c +++ b/lib/kunit/device.c @@ -119,10 +119,8 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, kunit_dev->owner = test;
err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name); - if (err) { - kfree(kunit_dev); - return ERR_PTR(err); - } + if (err) + goto error;
kunit_dev->dev.release = kunit_device_release; kunit_dev->dev.bus = &kunit_bus_type; @@ -131,7 +129,7 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, err = device_register(&kunit_dev->dev); if (err) { put_device(&kunit_dev->dev); - return ERR_PTR(err); + goto error; }
kunit_dev->dev.dma_mask = &kunit_dev->dev.coherent_dma_mask; @@ -140,6 +138,9 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
return kunit_dev; +error: + kfree(kunit_dev); + return ERR_PTR(err); }
/*
On Fri, 19 Apr 2024 at 05:02, Wander Lairson Costa wander@redhat.com wrote:
If the device register fails, free the allocated memory before returning.
Signed-off-by: Wander Lairson Costa wander@redhat.com Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
Thanks.
I'm not sure this is correct, though... Shouldn't put_device() free this for us?
The documentation for device_register() says to never free a device after device_register() has been called, even if it fails: https://docs.kernel.org/driver-api/infrastructure.html#c.device_register
Or am I missing something?
Cheers, -- David
lib/kunit/device.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/lib/kunit/device.c b/lib/kunit/device.c index 25c81ed465fb..bc2e2032e505 100644 --- a/lib/kunit/device.c +++ b/lib/kunit/device.c @@ -119,10 +119,8 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, kunit_dev->owner = test;
err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
if (err) {kfree(kunit_dev);return ERR_PTR(err);}
if (err)goto error; kunit_dev->dev.release = kunit_device_release; kunit_dev->dev.bus = &kunit_bus_type;@@ -131,7 +129,7 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, err = device_register(&kunit_dev->dev); if (err) { put_device(&kunit_dev->dev);
return ERR_PTR(err);
goto error; } kunit_dev->dev.dma_mask = &kunit_dev->dev.coherent_dma_mask;@@ -140,6 +138,9 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
return kunit_dev;+error:
kfree(kunit_dev);return ERR_PTR(err);}
/*
2.44.0
On Fri, Apr 19, 2024 at 1:59 AM David Gow davidgow@google.com wrote:
On Fri, 19 Apr 2024 at 05:02, Wander Lairson Costa wander@redhat.com wrote:
If the device register fails, free the allocated memory before returning.
Signed-off-by: Wander Lairson Costa wander@redhat.com Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
Thanks.
I'm not sure this is correct, though... Shouldn't put_device() free this for us?
The documentation for device_register() says to never free a device after device_register() has been called, even if it fails: https://docs.kernel.org/driver-api/infrastructure.html#c.device_register
Or am I missing something?
I am not freeing the device object passed to device_register, but its parent structure.
As a side note, the behavior of device_register() seems counterintuitive and error-prone, IMO. If the function returns an error, it should ensure it leaks no resource and shouldn't require the caller to do any cleanup.
Cheers, -- David
lib/kunit/device.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/lib/kunit/device.c b/lib/kunit/device.c index 25c81ed465fb..bc2e2032e505 100644 --- a/lib/kunit/device.c +++ b/lib/kunit/device.c @@ -119,10 +119,8 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, kunit_dev->owner = test;
err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
if (err) {kfree(kunit_dev);return ERR_PTR(err);}
if (err)goto error; kunit_dev->dev.release = kunit_device_release; kunit_dev->dev.bus = &kunit_bus_type;@@ -131,7 +129,7 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, err = device_register(&kunit_dev->dev); if (err) { put_device(&kunit_dev->dev);
return ERR_PTR(err);
goto error; } kunit_dev->dev.dma_mask = &kunit_dev->dev.coherent_dma_mask;@@ -140,6 +138,9 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
return kunit_dev;+error:
kfree(kunit_dev);return ERR_PTR(err);}
/*
2.44.0
On Fri, Apr 19, 2024 at 09:30:06AM -0300, Wander Lairson Costa wrote:
As a side note, the behavior of device_register() seems counterintuitive and error-prone, IMO. If the function returns an error, it should ensure it leaks no resource and shouldn't require the caller to do any cleanup.
I too want a pony, but that's not the way the code works here, sorry. It's always been like this, and has always been a problem, but last time I looked, there was no way to really fix this. That's why we document it a lot to make sure people don't get the error paths wrong here. I know it's a pain :(
sorry,
greg k-h
If the device register fails, free the allocated memory before returning.
Can a description variant (like the following) be more appropriate?
Free the allocated memory (after a device registration failure) before returning. Thus add a jump target so that a bit of exception handling can be better reused at the end of this function implementation.
Would you like to replace the word “register” by “registration” also in the summary phrase?
…
+++ b/lib/kunit/device.c
…
@@ -140,6 +138,9 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test, kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
return kunit_dev; +error:
- kfree(kunit_dev);
- return ERR_PTR(err);
}
…
I find it nicer to use a label like free_device.
Regards, Markus
On Fri, Apr 19, 2024 at 08:15:25AM +0200, Markus Elfring wrote:
If the device register fails, free the allocated memory before returning.
Can a description variant (like the following) be more appropriate?
Free the allocated memory (after a device registration failure) before returning. Thus add a jump target so that a bit of exception handling can be better reused at the end of this function implementation.
Would you like to replace the word “register” by “registration” also in the summary phrase?
Hi,
This is the semi-friendly patch-bot of Greg Kroah-Hartman.
Markus, you seem to have sent a nonsensical or otherwise pointless review comment to a patch submission on a Linux kernel developer mailing list. I strongly suggest that you not do this anymore. Please do not bother developers who are actively working to produce patches and features with comments that, in the end, are a waste of time.
Patch submitter, please ignore Markus's suggestion; you do not need to follow it at all. The person/bot/AI that sent it is being ignored by almost all Linux kernel maintainers for having a persistent pattern of behavior of producing distracting and pointless commentary, and inability to adapt to feedback. Please feel free to also ignore emails from them.
thanks,
greg k-h's patch email bot
linux-kselftest-mirror@lists.linaro.org
-
David Gow -
Greg Kroah-Hartman -
Markus Elfring -
Wander Lairson Costa