| Message ID | 20240419132504.9488-3-wander@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | kunit: fix minor error path mistakes | expand |
On Fri, Apr 19, 2024 at 10:25:02AM -0300, Wander Lairson Costa 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") > --- > 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); 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
On Fri, Apr 19, 2024 at 11:03 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Fri, Apr 19, 2024 at 10:25:02AM -0300, Wander Lairson Costa 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") > > --- > > 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); > > 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. > 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 >
On Fri, 19 Apr 2024 at 22:11, Wander Lairson Costa <wander@redhat.com> wrote: > > On Fri, Apr 19, 2024 at 11:03 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Fri, Apr 19, 2024 at 10:25:02AM -0300, Wander Lairson Costa 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") > > > --- > > > 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); > > > > 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. > > > > 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. 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
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); }
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 | 1 + 1 file changed, 1 insertion(+)