Re: [PATCH] staging: rtl8723bs: os_dep: simplify the return statement.

[Saurav Girepunje <saurav.girepunje@gmail.com>]

On 11/10/21 5:56 pm, Dan Carpenter wrote:
> This introduces a use after free on the sucess path.  You need to be a
> lot more careful.
> 
> On Sat, Oct 09, 2021 at 09:09:12PM +0530, Saurav Girepunje wrote:
>> Remove the unneeded and redundant check of variable on goto out.
>> Simplify the return using multiple goto label to avoid
>> unneeded check.
>>
>> Signed-off-by: Saurav Girepunje <saurav.girepunje@gmail.com>
>> ---
>>  .../staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 29 ++++++++++---------
>>  1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
>> index 0868f56e2979..574fdb6adce7 100644
>> --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
>> +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
>> @@ -2282,18 +2282,18 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
>>
>>  	if (!name) {
>>  		ret = -EINVAL;
>> -		goto out;
>> +		goto err_out;
> 
> Just return directly.  "return -EINVAL;" but what does "goto err_out;"
> do?  No one knows without scrolling down to the very bottom of the
> function, then scrolling all the way up again.  At this point you have
> lost your place in the code and your train of thought is de-railed.
> 
> Plus it introduces "forgot to set the error code" bugs.
> 
>> @@ -2312,7 +2312,7 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
>>  	mon_wdev = rtw_zmalloc(sizeof(struct wireless_dev));
>>  	if (!mon_wdev) {
>>  		ret = -ENOMEM;
>> -		goto out;
>> +		goto err_zmalloc;
> 
> 
> This is a Come From style naming.  Imagine if instead of naming functions
> after what they do we instead named them after the first caller which
> was introduced.  kmalloc() would be named called_from_boot_510().  It's
> a usless naming scheme.  We have to scroll down to the bottom to see
> what it does.
>
I will send another patch to remove goto and simply return ret value.
>>  	}
>>
>>  	mon_wdev->wiphy = padapter->rtw_wdev->wiphy;
>> @@ -2322,22 +2322,23 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
>>
>>  	ret = cfg80211_register_netdevice(mon_ndev);
>>  	if (ret) {
>> -		goto out;
>> +		goto err_register;
>>  	}
>>
>>  	*ndev = pwdev_priv->pmon_ndev = mon_ndev;
>>  	memcpy(pwdev_priv->ifname_mon, name, IFNAMSIZ+1);
>>
>> -out:
>> -	if (ret && mon_wdev) {
>> -		kfree(mon_wdev);
>> -		mon_wdev = NULL;
>> -	}
>> +err_register:
>>
>> -	if (ret && mon_ndev) {
>> -		free_netdev(mon_ndev);
>> -		*ndev = mon_ndev = NULL;
>> -	}
>> +	kfree(mon_wdev);
>> +	mon_wdev = NULL;
> 
> This is an on stack variable.  Think about what you are doing.  You're
> not writing carefully at all.
> 
I didn't removed local variable assignment to NULL on this patch. 
However I agree this is another improvement possible on this function and can be done along with other changes. 
Please let me know you opinion whether I should send one patch or multiple patch in a single series.
 
>> +
>> +err_zmalloc:
>> +
>> +	free_netdev(mon_ndev);
>> +	*ndev = mon_ndev = NULL;
> 
> mon_ndev is local too.
> 
> regards,
> dan carpenter
> 
> 
Regards,
Saurav