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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1661874.ovfstkzlfj@localhost.localdomain>
Date:   Sat, 09 Oct 2021 18:59:57 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     gregkh@...uxfoundation.org, linux-staging@...ts.linux.dev,
        Saurav Girepunje <saurav.girepunje@...il.com>
Cc:     fabioaiuto83@...il.com, ross.schm.dev@...il.com,
        marcocesati@...il.com, insafonov@...il.com,
        linux-kernel@...r.kernel.org, saurav.girepunje@...mail.com
Subject: Re: [PATCH] staging: rtl8723bs: os_dep: simplify the return statement.

On Saturday, October 9, 2021 5:39:12 PM CEST 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@...il.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;
>  	}
> 
>  	if (pwdev_priv->pmon_ndev) {
>  		ret = -EBUSY;
> -		goto out;
> +		goto err_out;
>  	}
> 
>  	mon_ndev = alloc_etherdev(sizeof(struct 
rtw_netdev_priv_indicator));
>  	if (!mon_ndev) {
>  		ret = -ENOMEM;
> -		goto out;
> +		goto err_out;
>  	}
> 
>  	mon_ndev->type = ARPHRD_IEEE80211_RADIOTAP;
> @@ -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;
>  	}
> 
>  	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;
> +
> +err_zmalloc:
> +
> +	free_netdev(mon_ndev);
> +	*ndev = mon_ndev = NULL;
> +
> +err_out:
> 
>  	return ret;
>  }
> --
> 2.32.0
> 
At a quick look it seems that now you unconditionally free memory and free a 
network device immediately after the last memcpy(). 

Why are you doing this even if no errors are returned from device and memory 
allocations (i.e., while "ret" is still equal to zero)?

Thanks,

Fabio