* [PATCH -next] staging: rtl8192e: rtllib_module: fix missing free_netdev() on error in alloc_rtllib() @ 2021-11-30 3:40 Yang Yingliang 2021-11-30 18:57 ` Pavel Skripkin 0 siblings, 1 reply; 5+ messages in thread From: Yang Yingliang @ 2021-11-30 3:40 UTC (permalink / raw) To: linux-kernel, linux-staging; +Cc: gregkh Add the missing free_netdev() before return from alloc_rtllib() in the error handling case. Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- drivers/staging/rtl8192e/rtllib_module.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/rtl8192e/rtllib_module.c b/drivers/staging/rtl8192e/rtllib_module.c index 64d9feee1f39..18d898714c5c 100644 --- a/drivers/staging/rtl8192e/rtllib_module.c +++ b/drivers/staging/rtl8192e/rtllib_module.c @@ -125,7 +125,7 @@ struct net_device *alloc_rtllib(int sizeof_priv) ieee->pHTInfo = kzalloc(sizeof(struct rt_hi_throughput), GFP_KERNEL); if (!ieee->pHTInfo) - return NULL; + goto failed; HTUpdateDefaultSetting(ieee); HTInitializeHTInfo(ieee); -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH -next] staging: rtl8192e: rtllib_module: fix missing free_netdev() on error in alloc_rtllib() 2021-11-30 3:40 [PATCH -next] staging: rtl8192e: rtllib_module: fix missing free_netdev() on error in alloc_rtllib() Yang Yingliang @ 2021-11-30 18:57 ` Pavel Skripkin 2021-12-01 2:41 ` Yang Yingliang 0 siblings, 1 reply; 5+ messages in thread From: Pavel Skripkin @ 2021-11-30 18:57 UTC (permalink / raw) To: Yang Yingliang, linux-kernel, linux-staging; +Cc: gregkh On 11/30/21 06:40, Yang Yingliang wrote: > Add the missing free_netdev() before return from alloc_rtllib() > in the error handling case. > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > drivers/staging/rtl8192e/rtllib_module.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/rtl8192e/rtllib_module.c b/drivers/staging/rtl8192e/rtllib_module.c > index 64d9feee1f39..18d898714c5c 100644 > --- a/drivers/staging/rtl8192e/rtllib_module.c > +++ b/drivers/staging/rtl8192e/rtllib_module.c > @@ -125,7 +125,7 @@ struct net_device *alloc_rtllib(int sizeof_priv) > > ieee->pHTInfo = kzalloc(sizeof(struct rt_hi_throughput), GFP_KERNEL); > if (!ieee->pHTInfo) > - return NULL; > + goto failed; > > HTUpdateDefaultSetting(ieee); > HTInitializeHTInfo(ieee); > Good catch! There are 2 more possible leaks, tho. rtllib_networks_allocate() and rtllib_softmac_init() should be unwinded too. For some odd reason rtllib_softmac_init() does not return an error in case of allocation failure, but it should be fixed. I think, it worth to fix whole error handling in one patch With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] staging: rtl8192e: rtllib_module: fix missing free_netdev() on error in alloc_rtllib() 2021-11-30 18:57 ` Pavel Skripkin @ 2021-12-01 2:41 ` Yang Yingliang 2021-12-01 6:55 ` Dan Carpenter 0 siblings, 1 reply; 5+ messages in thread From: Yang Yingliang @ 2021-12-01 2:41 UTC (permalink / raw) To: Pavel Skripkin, linux-kernel, linux-staging; +Cc: gregkh Hi, On 2021/12/1 2:57, Pavel Skripkin wrote: > On 11/30/21 06:40, Yang Yingliang wrote: >> Add the missing free_netdev() before return from alloc_rtllib() >> in the error handling case. >> >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >> --- >> drivers/staging/rtl8192e/rtllib_module.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/staging/rtl8192e/rtllib_module.c >> b/drivers/staging/rtl8192e/rtllib_module.c >> index 64d9feee1f39..18d898714c5c 100644 >> --- a/drivers/staging/rtl8192e/rtllib_module.c >> +++ b/drivers/staging/rtl8192e/rtllib_module.c >> @@ -125,7 +125,7 @@ struct net_device *alloc_rtllib(int sizeof_priv) >> ieee->pHTInfo = kzalloc(sizeof(struct rt_hi_throughput), >> GFP_KERNEL); >> if (!ieee->pHTInfo) >> - return NULL; >> + goto failed; >> HTUpdateDefaultSetting(ieee); >> HTInitializeHTInfo(ieee); >> > > Good catch! > > There are 2 more possible leaks, tho. rtllib_networks_allocate() and > rtllib_softmac_init() should be unwinded too. The error path of rtllib_networks_allocate() won't leak the dev. > > For some odd reason rtllib_softmac_init() does not return an error in > case of allocation failure, but it should be fixed. I think, it worth > to fix whole error handling in one patch I will send a v2 to fix this. Thanks, Yang > > > > > With regards, > Pavel Skripkin > . ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] staging: rtl8192e: rtllib_module: fix missing free_netdev() on error in alloc_rtllib() 2021-12-01 2:41 ` Yang Yingliang @ 2021-12-01 6:55 ` Dan Carpenter 2021-12-01 7:29 ` Yang Yingliang 0 siblings, 1 reply; 5+ messages in thread From: Dan Carpenter @ 2021-12-01 6:55 UTC (permalink / raw) To: Yang Yingliang; +Cc: Pavel Skripkin, linux-kernel, linux-staging, gregkh On Wed, Dec 01, 2021 at 10:41:41AM +0800, Yang Yingliang wrote: > Hi, > > On 2021/12/1 2:57, Pavel Skripkin wrote: > > On 11/30/21 06:40, Yang Yingliang wrote: > > > Add the missing free_netdev() before return from alloc_rtllib() > > > in the error handling case. > > > > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > > --- > > > drivers/staging/rtl8192e/rtllib_module.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/rtl8192e/rtllib_module.c > > > b/drivers/staging/rtl8192e/rtllib_module.c > > > index 64d9feee1f39..18d898714c5c 100644 > > > --- a/drivers/staging/rtl8192e/rtllib_module.c > > > +++ b/drivers/staging/rtl8192e/rtllib_module.c > > > @@ -125,7 +125,7 @@ struct net_device *alloc_rtllib(int sizeof_priv) > > > ieee->pHTInfo = kzalloc(sizeof(struct rt_hi_throughput), > > > GFP_KERNEL); > > > if (!ieee->pHTInfo) > > > - return NULL; > > > + goto failed; > > > HTUpdateDefaultSetting(ieee); > > > HTInitializeHTInfo(ieee); > > > > > > > Good catch! > > > > There are 2 more possible leaks, tho. rtllib_networks_allocate() and > > rtllib_softmac_init() should be unwinded too. > The error path of rtllib_networks_allocate() won't leak the dev. You've misunderstood what Pavel is saying. He's saying that we need to call rtllib_networks_free() as well as free_netdev(). This code has a "goto failed" and that means it is either going to do nothing or do everything. It is a bad style of error handling and it often has bugs like this. The best way to write error handling is to use Free the Last Thing style. int my_alloc_function() { a = alloc(); if (!a) return -ENOMEM; // <- there is no last thing b = alloc(); if (!b) { ret = -ENOMEM; goto free_a; // <- this name says "a" is the last thing } c = alloc(); if (!c) { ret = -ENOMEM; goto free_b; } return 0; free_b: free(b); free_a: free(a); return ret; } In this style of error handling you only need to remember the last successful allocation and the names tell you what the goto does so it is much easier to check if it's correct. Then to create a my_free_function() you can just: Copy and paste the error handling. Add a free(c). Delete the labels. void my_free_function() { free(c); free(b); free(a); } The free function for alloc_rtllib() is free_rtllib() and it looks like this: drivers/staging/rtl8192e/rtllib_module.c 150 void free_rtllib(struct net_device *dev) 151 { 152 struct rtllib_device *ieee = (struct rtllib_device *) 153 netdev_priv_rsl(dev); 154 155 kfree(ieee->pHTInfo); 156 ieee->pHTInfo = NULL; ^^^^^^^^^^^^^^^^^^^^^ This line is pointless and should be deleted. 157 rtllib_softmac_free(ieee); 158 159 lib80211_crypt_info_free(&ieee->crypt_info); 160 161 rtllib_networks_free(ieee); 162 free_netdev(dev); 163 } As you can see this free function calls rtllib_softmac_free(), lib80211_crypt_info_free() and rtllib_networks_free() so the error handling in alloc_rtllib() needs to do that as well. Based on what I have said, then ideally the error handling for alloc_rtllib() would look something like this: return dev; free_softmac: rtllib_softmac_free(ieee); lib80211_crypt_info_free(&ieee->crypt_info); free_networks: rtllib_networks_free(ieee); free_netdev: free_netdev(dev); return NULL; The rtllib_softmac_init() function should really return an int, but it returns void. That could be fixed in a separate patch if you want. regards, dan carpenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] staging: rtl8192e: rtllib_module: fix missing free_netdev() on error in alloc_rtllib() 2021-12-01 6:55 ` Dan Carpenter @ 2021-12-01 7:29 ` Yang Yingliang 0 siblings, 0 replies; 5+ messages in thread From: Yang Yingliang @ 2021-12-01 7:29 UTC (permalink / raw) To: Dan Carpenter; +Cc: Pavel Skripkin, linux-kernel, linux-staging, gregkh Hi, On 2021/12/1 14:55, Dan Carpenter wrote: > On Wed, Dec 01, 2021 at 10:41:41AM +0800, Yang Yingliang wrote: >> Hi, >> >> On 2021/12/1 2:57, Pavel Skripkin wrote: >>> On 11/30/21 06:40, Yang Yingliang wrote: >>>> Add the missing free_netdev() before return from alloc_rtllib() >>>> in the error handling case. >>>> >>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >>>> --- >>>> drivers/staging/rtl8192e/rtllib_module.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/staging/rtl8192e/rtllib_module.c >>>> b/drivers/staging/rtl8192e/rtllib_module.c >>>> index 64d9feee1f39..18d898714c5c 100644 >>>> --- a/drivers/staging/rtl8192e/rtllib_module.c >>>> +++ b/drivers/staging/rtl8192e/rtllib_module.c >>>> @@ -125,7 +125,7 @@ struct net_device *alloc_rtllib(int sizeof_priv) >>>> ieee->pHTInfo = kzalloc(sizeof(struct rt_hi_throughput), >>>> GFP_KERNEL); >>>> if (!ieee->pHTInfo) >>>> - return NULL; >>>> + goto failed; >>>> HTUpdateDefaultSetting(ieee); >>>> HTInitializeHTInfo(ieee); >>>> >>> Good catch! >>> >>> There are 2 more possible leaks, tho. rtllib_networks_allocate() and >>> rtllib_softmac_init() should be unwinded too. >> The error path of rtllib_networks_allocate() won't leak the dev. > You've misunderstood what Pavel is saying. He's saying that we need to > call rtllib_networks_free() as well as free_netdev(). > > This code has a "goto failed" and that means it is either going to do > nothing or do everything. It is a bad style of error handling and it > often has bugs like this. The best way to write error handling is to > use Free the Last Thing style. > > int my_alloc_function() > { > a = alloc(); > if (!a) > return -ENOMEM; // <- there is no last thing > > b = alloc(); > if (!b) { > ret = -ENOMEM; > goto free_a; // <- this name says "a" is the last thing > } > > c = alloc(); > if (!c) { > ret = -ENOMEM; > goto free_b; > } > > return 0; > > free_b: > free(b); > free_a: > free(a); > > return ret; > } > > In this style of error handling you only need to remember the last > successful allocation and the names tell you what the goto does so it > is much easier to check if it's correct. > > Then to create a my_free_function() you can just: Copy and paste the > error handling. Add a free(c). Delete the labels. > > void my_free_function() > { > free(c); > free(b); > free(a); > } > > The free function for alloc_rtllib() is free_rtllib() and it looks like > this: > > drivers/staging/rtl8192e/rtllib_module.c > 150 void free_rtllib(struct net_device *dev) > 151 { > 152 struct rtllib_device *ieee = (struct rtllib_device *) > 153 netdev_priv_rsl(dev); > 154 > 155 kfree(ieee->pHTInfo); > 156 ieee->pHTInfo = NULL; > ^^^^^^^^^^^^^^^^^^^^^ > This line is pointless and should be deleted. > > 157 rtllib_softmac_free(ieee); > 158 > 159 lib80211_crypt_info_free(&ieee->crypt_info); > 160 > 161 rtllib_networks_free(ieee); > 162 free_netdev(dev); > 163 } > > As you can see this free function calls rtllib_softmac_free(), > lib80211_crypt_info_free() and rtllib_networks_free() so the error > handling in alloc_rtllib() needs to do that as well. Based on what > I have said, then ideally the error handling for alloc_rtllib() would > look something like this: > > return dev; > > free_softmac: > rtllib_softmac_free(ieee); > lib80211_crypt_info_free(&ieee->crypt_info); > free_networks: > rtllib_networks_free(ieee); > free_netdev: > free_netdev(dev); > > return NULL; > > The rtllib_softmac_init() function should really return an int, but it > returns void. That could be fixed in a separate patch if you want. I get it, thanks for your suggestion. > > regards, > dan carpenter > . ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-12-01 7:30 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-30 3:40 [PATCH -next] staging: rtl8192e: rtllib_module: fix missing free_netdev() on error in alloc_rtllib() Yang Yingliang 2021-11-30 18:57 ` Pavel Skripkin 2021-12-01 2:41 ` Yang Yingliang 2021-12-01 6:55 ` Dan Carpenter 2021-12-01 7:29 ` Yang Yingliang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).