2022-11-04 09:13:27

by Jinjie Ruan

Subject: [PATCH] vdpa_sim: fix possible memory leak in vdpasim_net_init() and vdpasim_blk_init()

If device_register() returns error in vdpasim_net_init() or
vdpasim_blk_init(), name of kobject which is allocated in dev_set_name()
called in device_add() is leaked.

As comment of device_add() says, it should call put_device() to drop
the reference count that was set in device_initialize() when it fails,
so the name can be freed in kobject_cleanup().

Signed-off-by: ruanjinjie <[email protected]>
---
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 4 +++-
drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index c6db1a1baf76..f745926237a8 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -427,8 +427,10 @@ static int __init vdpasim_blk_init(void)
int ret;

ret = device_register(&vdpasim_blk_mgmtdev);
- if (ret)
+ if (ret) {
+ put_device(&vdpasim_blk_mgmtdev);
return ret;
+ }

ret = vdpa_mgmtdev_register(&mgmt_dev);
if (ret)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
index c3cb225ea469..11f5a121df24 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -305,8 +305,10 @@ static int __init vdpasim_net_init(void)
int ret;

ret = device_register(&vdpasim_net_mgmtdev);
- if (ret)
+ if (ret) {
+ put_device(&vdpasim_net_mgmtdev);
return ret;
+ }

ret = vdpa_mgmtdev_register(&mgmt_dev);
if (ret)
--
2.25.1

2022-11-07 08:29:56

by Jason Wang

Subject: Re: [PATCH] vdpa_sim: fix possible memory leak in vdpasim_net_init() and vdpasim_blk_init()

On Fri, Nov 4, 2022 at 4:41 PM ruanjinjie <[email protected]> wrote:
>
> If device_register() returns error in vdpasim_net_init() or
> vdpasim_blk_init(), name of kobject which is allocated in dev_set_name()
> called in device_add() is leaked.
>
> As comment of device_add() says, it should call put_device() to drop
> the reference count that was set in device_initialize() when it fails,
> so the name can be freed in kobject_cleanup().
>
> Signed-off-by: ruanjinjie <[email protected]>

Acked-by: Jason Wang <[email protected]>

Should we have a fixes tag and cc stable?

Thanks

> ---
> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 4 +++-
> drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 4 +++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> index c6db1a1baf76..f745926237a8 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> @@ -427,8 +427,10 @@ static int __init vdpasim_blk_init(void)
> int ret;
>
> ret = device_register(&vdpasim_blk_mgmtdev);
> - if (ret)
> + if (ret) {
> + put_device(&vdpasim_blk_mgmtdev);
> return ret;
> + }
>
> ret = vdpa_mgmtdev_register(&mgmt_dev);
> if (ret)
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> index c3cb225ea469..11f5a121df24 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> @@ -305,8 +305,10 @@ static int __init vdpasim_net_init(void)
> int ret;
>
> ret = device_register(&vdpasim_net_mgmtdev);
> - if (ret)
> + if (ret) {
> + put_device(&vdpasim_net_mgmtdev);
> return ret;
> + }
>
> ret = vdpa_mgmtdev_register(&mgmt_dev);
> if (ret)
> --
> 2.25.1
>

2022-11-07 09:19:18

by Stefano Garzarella

Subject: Re: [PATCH] vdpa_sim: fix possible memory leak in vdpasim_net_init() and vdpasim_blk_init()

On Mon, Nov 07, 2022 at 03:41:42PM +0800, Jason Wang wrote:
>On Fri, Nov 4, 2022 at 4:41 PM ruanjinjie <[email protected]> wrote:
>>
>> If device_register() returns error in vdpasim_net_init() or
>> vdpasim_blk_init(), name of kobject which is allocated in dev_set_name()
>> called in device_add() is leaked.
>>
>> As comment of device_add() says, it should call put_device() to drop
>> the reference count that was set in device_initialize() when it fails,
>> so the name can be freed in kobject_cleanup().
>>
>> Signed-off-by: ruanjinjie <[email protected]>
>
>Acked-by: Jason Wang <[email protected]>
>
>Should we have a fixes tag and cc stable?

Yep, I think so.

The fixes tags should be:

Fixes: 899c4d187f6a ("vdpa_sim_blk: add support for vdpa management tool")
Fixes: a3c06ae158dd ("vdpa_sim_net: Add support for user supported devices")

With them:

Reviewed-by: Stefano Garzarella <[email protected]>

Thanks,
Stefano