[PATCH 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.

Wander Lairson Costa (2):
kunit: unregister the device on error
kunit: avoid memory leak on device register error

lib/kunit/device.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

--
2.44.0

[Wander Lairson Costa]

kunit_init_device() should unregister the device on bus register error.

Signed-off-by: Wander Lairson Costa <wan...@redhat.com>
---
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

[Wander Lairson Costa]

If the device register fails, free the allocated memory before
returning.

Signed-off-by: Wander Lairson Costa <wan...@redhat.com>
---

lib/kunit/device.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/lib/kunit/device.c b/lib/kunit/device.c
index 25c81ed465fb..d8c09dcb3e79 100644
--- a/lib/kunit/device.c
+++ b/lib/kunit/device.c
@@ -131,6 +131,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);
+ kfree(kunit_dev);
return ERR_PTR(err);
}

--
2.44.0

[Ivan Orlov]

Maybe it worth adding a 'Fixes' tag, since it is definitely a bug.
--
Kind regards,
Ivan Orlov

[Markus Elfring]

> kunit_init_device() should unregister the device on bus register error.

* Would another imperative wording be desirable also for this change description?

* Will the tag “Fixes” become relevant here?

Regards,
Markus

[Wander Lairson Costa]

On Thu, Apr 18, 2024 at 12:06 PM Markus Elfring <Markus....@web.de> wrote:
>
> > kunit_init_device() should unregister the device on bus register error.
>


> * Would another imperative wording be desirable also for this change description?

It makes sense, I will change the comment description.

>
> * Will the tag “Fixes” become relevant here?

I often forget this tag. I will add it.

>
> Regards,
> Markus
>

[Markus Elfring]

> If the device register fails, free the allocated memory before
> returning.

* I suggest to use the word “registration” (instead of “register”)
in the commit message.

* Would you like to add the tag “Fixes” accordingly?

> +++ b/lib/kunit/device.c
> @@ -131,6 +131,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);
> + kfree(kunit_dev);
> return ERR_PTR(err);
> }

Common error handling code can be used instead
if an additional label would be applied for a corresponding jump target.

How do you think about to increase the application of scope-based resource management here?

Regards,
Markus

[Wander Lairson Costa]

Hi,

These two patches fix some minor error path mistakes in the device
module.

Changes:
--------

v1->v2:
* Add fixes tag.
* Add an imperative statement in the first commit descripton.

[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 <wan...@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

[Wander Lairson Costa]

If the device register fails, free the allocated memory before
returning.

Signed-off-by: Wander Lairson Costa <wan...@redhat.com>
Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
---

lib/kunit/device.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/lib/kunit/device.c b/lib/kunit/device.c
index 25c81ed465fb..d8c09dcb3e79 100644
--- a/lib/kunit/device.c
+++ b/lib/kunit/device.c

@@ -131,6 +131,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);
+ kfree(kunit_dev);
return ERR_PTR(err);
}

--
2.44.0

[Maxime Ripard]

On Thu, 18 Apr 2024 13:00:36 -0300, Wander Lairson Costa wrote:
> Hi,
>
> These two patches fix some minor error path mistakes in the device
> module.
>
>

> [ ... ]

Reviewed-by: Maxime Ripard <mri...@kernel.org>

Thanks!
Maxime

[Wander Lairson Costa]

I thought about that. But I think the code is simple enough (for now)
to not require an exit label.

> Regards,
> Markus
>

[Markus Elfring]

>> Common error handling code can be used instead
>> if an additional label would be applied for a corresponding jump target.
>>
>> How do you think about to increase the application of scope-based resource management here?
>
> I thought about that. But I think the code is simple enough (for now)
> to not require an exit label.

Please follow a known advice (besides other recommended improvements).
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources

Regards,
Markus

[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

[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 <wan...@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

[Wander Lairson Costa]

If the device register fails, free the allocated memory before
returning.

Signed-off-by: Wander Lairson Costa <wan...@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

[David Gow]

On Fri, 19 Apr 2024 at 05:02, Wander Lairson Costa <wan...@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 <wan...@redhat.com>
> Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
> ---

Nice catch!

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

[David Gow]

On Fri, 19 Apr 2024 at 05:02, Wander Lairson Costa <wan...@redhat.com> wrote:
>

> If the device register fails, free the allocated memory before
> returning.
>
> Signed-off-by: Wander Lairson Costa <wan...@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

[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

[Greg Kroah-Hartman]

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

[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

[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?
>

[Wander Lairson Costa]

On Fri, Apr 19, 2024 at 1:59 AM David Gow <davi...@google.com> wrote:
>
> On Fri, 19 Apr 2024 at 05:02, Wander Lairson Costa <wan...@redhat.com> wrote:
> >
> > If the device register fails, free the allocated memory before
> > returning.
> >
> > Signed-off-by: Wander Lairson Costa <wan...@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.

[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

v3->v4
* Remove some changes requested by Marcus Elfring, as I was alerted he
is a known troll.

[Wander Lairson Costa]

If the device register fails, free the allocated memory before
returning.

Signed-off-by: Wander Lairson Costa <wan...@redhat.com>
Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
---

lib/kunit/device.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/lib/kunit/device.c b/lib/kunit/device.c
index 25c81ed465fb..d8c09dcb3e79 100644
--- a/lib/kunit/device.c
+++ b/lib/kunit/device.c
@@ -131,6 +131,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);

[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 <wan...@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

[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

[Greg Kroah-Hartman]

This still looks wrong, the release function for the device should free
the memory here, not this kfree, as the reference count in the embedded
'struct device' handles the memory logic for the whole structure (if
not, then something is REALLY wrong...)

You _do_ have a release function for the device, right? If not, you
should be getting loud messages in the kernel log when releasing a
device here.

thanks,

greg k-h

[Greg Kroah-Hartman]

On Fri, Apr 19, 2024 at 10:25:01AM -0300, Wander Lairson Costa 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 <wan...@redhat.com>
> Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")

Reviewed-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

[Wander Lairson Costa]

Ok, I got it. Yes, there is a release function. So this patch is
wrong, ignore it. Should I send a v5 with only the other patch?

> thanks,
>
> greg k-h
>

[Markus Elfring]

…
> * Remove some changes requested by Marcus Elfring,

I became curious how affected software components can evolve further.

> as I was alerted he is a known troll.

I would appreciate if this interpretation will be reconsidered somehow.

Regards,
Markus

[Markus Elfring]

> > 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.

…
> Reviewed-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

Would you ever like to distinguish hardware register errors from
item registration failures according to further improved commit messages?

Regards,
Markus

[Greg Kroah-Hartman]

[David Gow]

Thanks. Don't worry about sending a v5: just patch 1 of v4 is now in
the kunit branch:
https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=63761ec5971ea47c1f2d7698f03e1c6ffb9fb22a

Cheers,
-- David