lkml.org 
[lkml]   [2021]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH -next] staging: rtl8192e: rtllib_module: fix missing free_netdev() on error in alloc_rtllib()
From
Date
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
> .

\
 
 \ /
  Last update: 2021-12-01 08:30    [W:0.071 / U:1.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site