| Message ID | 20210610152925.18145-1-find.dhiraj@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | function mana_hwc_create_wq leaks memory | expand |
> From: Dhiraj Shah <find.dhiraj@gmail.com> > Sent: Thursday, June 10, 2021 8:29 AM > ... > memory space allocated for the queue in function > mana_gd_create_hwc_queue is not freed during error condition. > > Signed-off-by: Dhiraj Shah <find.dhiraj@gmail.com> > --- > drivers/net/ethernet/microsoft/mana/hw_channel.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > index 1a923fd99990..4aa4bda518fb 100644 > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c > @@ -501,8 +501,10 @@ static int mana_hwc_create_wq(struct > hw_channel_context *hwc, > *hwc_wq_ptr = hwc_wq; > return 0; > out: > - if (err) > + if (err) { Here the 'err' must be non-zero. Can you please remove this 'if'? > + kfree(queue); > mana_hwc_destroy_wq(hwc, hwc_wq); > + } > return err; > } Reviewed-by: Dexuan Cui <decui@microsoft.com>
Hi Dexuan, Thanks for the feedback. You are right saying ‘mana_hwc_destroy_wq' free’s up the queue. However for example if function 'mana_hwc_alloc_dma_buf' fails it will goto ‘out' and call ‘mana_hwc_destroy_wq', the value 'hwc_wq->gdma_wq' is still not assigned at this point. In the ‘mana_hwc_destroy_wq' function i see it checks for 'hwc_wq->gdma_wq' before calling, ‘mana_gd_destroy_queue', which makes me think queue is still not freed. Please let me know if i am missing something. Regards, /Dhiraj > On 10 Jun 2021, at 18:28, Dexuan Cui <decui@microsoft.com> wrote: > >> From: Dexuan Cui <decui@microsoft.com> >> Sent: Thursday, June 10, 2021 10:18 AM >> ... >>> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c >>> b/drivers/net/ethernet/microsoft/mana/hw_channel.c >>> index 1a923fd99990..4aa4bda518fb 100644 >>> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c >>> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c >>> @@ -501,8 +501,10 @@ static int mana_hwc_create_wq(struct >>> hw_channel_context *hwc, >>> *hwc_wq_ptr = hwc_wq; >>> return 0; >>> out: >>> - if (err) >>> + if (err) { >> >> Here the 'err' must be non-zero. Can you please remove this 'if'? >> >>> + kfree(queue); >>> mana_hwc_destroy_wq(hwc, hwc_wq); >>> + } >>> return err; >>> } >> >> Reviewed-by: Dexuan Cui <decui@microsoft.com> > > Hi Dhiraj, > I checked the code again and it looks like your patch is actually > unnecessary as IMO there is no memory leak here: the 'queue' > pointer is passed to mana_hwc_destroy_wq() as hwc_wq->gdma_wq, > and is later freed in mana_gd_destroy_queue() -> > mana_gd_destroy_queue(). > > The 'if' test can be removed as the 'err's is always non-zero there. > > Thanks, > Dexuan
> From: Dhiraj Shah <find.dhiraj@gmail.com> > Sent: Thursday, June 10, 2021 12:53 PM > ... > Hi Dexuan, > > Thanks for the feedback. > > You are right saying ‘mana_hwc_destroy_wq' free’s up the queue. > > However for example if function 'mana_hwc_alloc_dma_buf' fails it will goto > ‘out' and call ‘mana_hwc_destroy_wq', the value 'hwc_wq->gdma_wq' is > still not assigned at this point. In the ‘mana_hwc_destroy_wq' function i see At this point, hwc_wq->gdma_wq stays with its default value NULL. > it checks for 'hwc_wq->gdma_wq' before calling, ‘mana_gd_destroy_queue', > which makes me think queue is still not freed. IMO the current code is not buggy, though I admit it's not very readable. :-) If you're interested, you're welcome to help make it more readable. I'll also try to make some time on this. Thanks, Dexuan
diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c index 1a923fd99990..4aa4bda518fb 100644 --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c @@ -501,8 +501,10 @@ static int mana_hwc_create_wq(struct hw_channel_context *hwc, *hwc_wq_ptr = hwc_wq; return 0; out: - if (err) + if (err) { + kfree(queue); mana_hwc_destroy_wq(hwc, hwc_wq); + } return err; }
memory space allocated for the queue in function mana_gd_create_hwc_queue is not freed during error condition. Signed-off-by: Dhiraj Shah <find.dhiraj@gmail.com> --- drivers/net/ethernet/microsoft/mana/hw_channel.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)