1. Patchew
  2. linux
  3. [PATCH v3 0/2] kunit: fix minor error path mistakes
  4. View patch

[PATCH v3 2/2] kunit: avoid memory leak on device register error

Wander Lairson Costa posted 2 patches 1 year, 10 months ago
There is a newer version of this series
[PATCH v3 2/2] kunit: avoid memory leak on device register error
Posted by Wander Lairson Costa 1 year, 10 months ago 
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);
 }
 
 /*
-- 
2.44.0
Re: [PATCH v3 2/2] kunit: avoid memory leak on device register error
Posted by Markus Elfring 1 year, 10 months ago 
> 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
Re: [PATCH v3 2/2] kunit: avoid memory leak on device register error
Posted by Greg Kroah-Hartman 1 year, 10 months ago 
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
Re: [PATCH v3 2/2] kunit: avoid memory leak on device register error
Posted by David Gow 1 year, 10 months ago 
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
>
Re: [PATCH v3 2/2] kunit: avoid memory leak on device register error
Posted by Wander Lairson Costa 1 year, 10 months ago 
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
> >
Re: [PATCH v3 2/2] kunit: avoid memory leak on device register error
Posted by Greg Kroah-Hartman 1 year, 10 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.