If the device register fails, free the allocated memory before
returning.
Signed-off-by: Wander Lairson Costa <[email protected]>
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);
}
/*
--
2.44.0
On Fri, 19 Apr 2024 at 05:02, Wander Lairson Costa <[email protected]> wrote:
>
> If the device register fails, free the allocated memory before
> returning.
>
> Signed-off-by: Wander Lairson Costa <[email protected]>
> 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
>
> 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
On Fri, Apr 19, 2024 at 1:59 AM David Gow <[email protected]> wrote:
>
> On Fri, 19 Apr 2024 at 05:02, Wander Lairson Costa <[email protected]> wrote:
> >
> > If the device register fails, free the allocated memory before
> > returning.
> >
> > Signed-off-by: Wander Lairson Costa <[email protected]>
> > 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