Re: [PATCH -next] staging: rtl8192e: rtllib_module: fix missing free_netdev() on error in alloc_rtllib()

[Yang Yingliang <yangyingliang@huawei.com>]

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
> .