* [PATCH] staging: rtl8723bs: os_dep: simplify the return statement.
@ 2021-10-09 15:39 Saurav Girepunje
2021-10-09 16:59 ` Fabio M. De Francesco
2021-10-11 12:26 ` Dan Carpenter
0 siblings, 2 replies; 6+ messages in thread
From: Saurav Girepunje @ 2021-10-09 15:39 UTC (permalink / raw)
To: gregkh, fabioaiuto83, ross.schm.dev, marcocesati, insafonov,
linux-staging, linux-kernel
Cc: saurav.girepunje
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;
}
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: os_dep: simplify the return statement.
2021-10-09 15:39 [PATCH] staging: rtl8723bs: os_dep: simplify the return statement Saurav Girepunje
@ 2021-10-09 16:59 ` Fabio M. De Francesco
2021-10-09 18:17 ` Saurav Girepunje
2021-10-11 12:26 ` Dan Carpenter
1 sibling, 1 reply; 6+ messages in thread
From: Fabio M. De Francesco @ 2021-10-09 16:59 UTC (permalink / raw)
To: gregkh, linux-staging, Saurav Girepunje
Cc: fabioaiuto83, ross.schm.dev, marcocesati, insafonov,
linux-kernel, saurav.girepunje
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@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;
> }
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: os_dep: simplify the return statement.
2021-10-09 16:59 ` Fabio M. De Francesco
@ 2021-10-09 18:17 ` Saurav Girepunje
0 siblings, 0 replies; 6+ messages in thread
From: Saurav Girepunje @ 2021-10-09 18:17 UTC (permalink / raw)
To: Fabio M. De Francesco, gregkh, linux-staging
Cc: saurav.girepunje, fabioaiuto83, ross.schm.dev, marcocesati,
insafonov, linux-kernel
On 09/10/21 10:29 pm, Fabio M. De Francesco wrote:
> 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@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;
>> }
>>
>> 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
>
>
>
>
Thanks Fabio for review. Yes, That is missed.
I can do that in either one of the way.
Either I should check for ret before free or I should use goto out(may
be I will rename err_out goto label to "out" only) just after the memcpy
statement so that function will not free for no error case and return
ret value only.
Please let me know you input on above.
Regards,
Saurav Girepunje
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: os_dep: simplify the return statement.
2021-10-09 15:39 [PATCH] staging: rtl8723bs: os_dep: simplify the return statement Saurav Girepunje
2021-10-09 16:59 ` Fabio M. De Francesco
@ 2021-10-11 12:26 ` Dan Carpenter
2021-10-11 18:26 ` Saurav Girepunje
1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-10-11 12:26 UTC (permalink / raw)
To: Saurav Girepunje
Cc: gregkh, fabioaiuto83, ross.schm.dev, marcocesati, insafonov,
linux-staging, linux-kernel, saurav.girepunje
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.
> }
>
> 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.
> +
> +err_zmalloc:
> +
> + free_netdev(mon_ndev);
> + *ndev = mon_ndev = NULL;
mon_ndev is local too.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: os_dep: simplify the return statement.
2021-10-11 12:26 ` Dan Carpenter
@ 2021-10-11 18:26 ` Saurav Girepunje
2021-10-12 6:34 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Saurav Girepunje @ 2021-10-11 18:26 UTC (permalink / raw)
To: Dan Carpenter
Cc: gregkh, fabioaiuto83, ross.schm.dev, marcocesati, insafonov,
linux-staging, linux-kernel, saurav.girepunje
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: rtl8723bs: os_dep: simplify the return statement.
2021-10-11 18:26 ` Saurav Girepunje
@ 2021-10-12 6:34 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2021-10-12 6:34 UTC (permalink / raw)
To: Saurav Girepunje
Cc: gregkh, fabioaiuto83, ross.schm.dev, marcocesati, insafonov,
linux-staging, linux-kernel, saurav.girepunje
On Mon, Oct 11, 2021 at 11:56:10PM +0530, Saurav Girepunje wrote:
>
> 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.
Since it's on the same line then do it in one patch.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-12 6:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-09 15:39 [PATCH] staging: rtl8723bs: os_dep: simplify the return statement Saurav Girepunje
2021-10-09 16:59 ` Fabio M. De Francesco
2021-10-09 18:17 ` Saurav Girepunje
2021-10-11 12:26 ` Dan Carpenter
2021-10-11 18:26 ` Saurav Girepunje
2021-10-12 6:34 ` Dan Carpenter
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).