[PATCH v3 0/2] kunit: fix minor error path mistakes

[PATCH v3 0/2] kunit: fix minor error path mistakes

[Wander Lairson Costa]

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(-)

-- 
2.44.0

[PATCH v3 1/2] kunit: unregister the device on error

[Wander Lairson Costa]

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;
 }
 
-- 
2.44.0

Re: [PATCH v3 1/2] kunit: unregister the device on error

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 963 bytes --]

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
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]

Re: [PATCH v3 1/2] kunit: unregister the device on error

[Markus Elfring]

> 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

Re: [PATCH v3 1/2] kunit: unregister the device on error

[Greg Kroah-Hartman]

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

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

[Wander Lairson Costa]

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

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]

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
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]

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

[Wander Lairson Costa]

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

[Greg Kroah-Hartman]

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

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

[Markus Elfring]

> 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

[Greg Kroah-Hartman]

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