| Message ID | 20230928091636.1209914-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] fpga: region: Fix possible memory leak in fpga_region_register_full() | expand |
On Thu, Sep 28, 2023 at 05:16:36PM +0800, Jinjie Ruan wrote: > If device_register() fails in fpga_region_register_full(), the region > allocated by kzalloc() and the id allocated by ida_alloc() also need be > freed otherwise will cause memory leak. How did you observe the memory leak? Please help provide some trace. Thanks, Yilun > > Fixes: 8886a579744f ("fpga: region: Use standard dev_release for class driver") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- > drivers/fpga/fpga-region.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c > index b364a929425c..9dc6314976ef 100644 > --- a/drivers/fpga/fpga-region.c > +++ b/drivers/fpga/fpga-region.c > @@ -228,12 +228,13 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info * > > ret = device_register(®ion->dev); > if (ret) { > - put_device(®ion->dev); > - return ERR_PTR(ret); > + goto err_put_device; > } > > return region; > > +err_put_device: > + put_device(®ion->dev); > err_remove: > ida_free(&fpga_region_ida, id); > err_free: > -- > 2.34.1 >
On 9/28/23 02:16, Jinjie Ruan wrote: > If device_register() fails in fpga_region_register_full(), the region > allocated by kzalloc() and the id allocated by ida_alloc() also need be > freed otherwise will cause memory leak. > > Fixes: 8886a579744f ("fpga: region: Use standard dev_release for class driver") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- > drivers/fpga/fpga-region.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c > index b364a929425c..9dc6314976ef 100644 > --- a/drivers/fpga/fpga-region.c > +++ b/drivers/fpga/fpga-region.c > @@ -228,12 +228,13 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info * > > ret = device_register(®ion->dev); The comments for device_register() say: * NOTE: _Never_ directly free @dev after calling this function, even * if it returned an error! Always use put_device() to give up the * reference initialized in this function instead. Note that dev is embedded in the region structure, so freeing region means freeing dev. The expectation is that after device_register() has been called, even if it returns an error, put_device() is used to lower the reference count. When the reference count is zero, the fpga_region_dev_release() function is called. fpga_region_dev_release() frees calls ida_free() and frees the region. Have you observed different behavior? Do you have evidence of a memory leak? Thanks, - Russ > if (ret) { > - put_device(®ion->dev); > - return ERR_PTR(ret); > + goto err_put_device; > } > > return region; > > +err_put_device: > + put_device(®ion->dev); > err_remove: > ida_free(&fpga_region_ida, id); > err_free:
On 2023/9/28 23:45, Russ Weight wrote: > > > On 9/28/23 02:16, Jinjie Ruan wrote: >> If device_register() fails in fpga_region_register_full(), the region >> allocated by kzalloc() and the id allocated by ida_alloc() also need be >> freed otherwise will cause memory leak. >> >> Fixes: 8886a579744f ("fpga: region: Use standard dev_release for class driver") >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >> --- >> drivers/fpga/fpga-region.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c >> index b364a929425c..9dc6314976ef 100644 >> --- a/drivers/fpga/fpga-region.c >> +++ b/drivers/fpga/fpga-region.c >> @@ -228,12 +228,13 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info * >> >> ret = device_register(®ion->dev); > > The comments for device_register() say: > > * NOTE: _Never_ directly free @dev after calling this function, even > * if it returned an error! Always use put_device() to give up the > * reference initialized in this function instead. > > Note that dev is embedded in the region structure, so freeing region > means freeing dev. > > The expectation is that after device_register() has been called, even > if it returns an error, put_device() is used to lower the reference > count. When the reference count is zero, the fpga_region_dev_release() > function is called. fpga_region_dev_release() frees calls ida_free() > and frees the region. Right! > > Have you observed different behavior? Do you have evidence of a memory > leak? I have noticed a memory leak in using fpga_region_register_full() in fpga-region-test.c. I'll send the patch sooner. > > Thanks, > - Russ > >> if (ret) { >> - put_device(®ion->dev); >> - return ERR_PTR(ret); >> + goto err_put_device; >> } >> >> return region; >> >> +err_put_device: >> + put_device(®ion->dev); >> err_remove: >> ida_free(&fpga_region_ida, id); >> err_free: >
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index b364a929425c..9dc6314976ef 100644 --- a/drivers/fpga/fpga-region.c +++ b/drivers/fpga/fpga-region.c @@ -228,12 +228,13 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info * ret = device_register(®ion->dev); if (ret) { - put_device(®ion->dev); - return ERR_PTR(ret); + goto err_put_device; } return region; +err_put_device: + put_device(®ion->dev); err_remove: ida_free(&fpga_region_ida, id); err_free:
If device_register() fails in fpga_region_register_full(), the region allocated by kzalloc() and the id allocated by ida_alloc() also need be freed otherwise will cause memory leak. Fixes: 8886a579744f ("fpga: region: Use standard dev_release for class driver") Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- drivers/fpga/fpga-region.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)