[PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail

[PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail

[Kate Hsuan]

Since _SUCCESS (1) and _FAIL (0) are used to indicate the status of the
functions. The previous commit 8ae7bf782eacad803f752c83a183393b0a67127b
fixed and prevented dereferencing a NULL pointer through checking the
return pointer. The NULL pointer check work properly but the return
values (-ENOMEM on fail and 0 on success). This work fixed the return
values to make sure the caller function will return the correct status.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2097526
Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/staging/r8188eu/core/rtw_xmit.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
index f4e9f6102539..2f8720db21d9 100644
--- a/drivers/staging/r8188eu/core/rtw_xmit.c
+++ b/drivers/staging/r8188eu/core/rtw_xmit.c
@@ -180,10 +180,8 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
 	pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
 
 	res = rtw_alloc_hwxmits(padapter);
-	if (res) {
-		res = _FAIL;
+	if (res == _FAIL)
 		goto exit;
-	}
 
 	rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
 
@@ -1510,7 +1508,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
 
 	pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
 	if (!pxmitpriv->hwxmits)
-		return -ENOMEM;
+		return _FAIL;
 
 	hwxmits = pxmitpriv->hwxmits;
 
@@ -1528,7 +1526,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
 	} else {
 	}
 
-	return 0;
+	return _SUCCESS;
 }
 
 void rtw_free_hwxmits(struct adapter *padapter)
-- 
2.36.1

Re: [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail

[Greg Kroah-Hartman]

On Mon, Jun 20, 2022 at 04:54:13PM +0800, Kate Hsuan wrote:
> Since _SUCCESS (1) and _FAIL (0) are used to indicate the status of the
> functions. The previous commit 8ae7bf782eacad803f752c83a183393b0a67127b

In the future, please reference commit ids as the documentation asks, as
it's a much nicer way to understand things.  Also this commit id is not
in Linus's tree or any tree that I can see, where did it come from?

> fixed and prevented dereferencing a NULL pointer through checking the
> return pointer. The NULL pointer check work properly but the return
> values (-ENOMEM on fail and 0 on success). This work fixed the return
> values to make sure the caller function will return the correct status.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2097526
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  drivers/staging/r8188eu/core/rtw_xmit.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> index f4e9f6102539..2f8720db21d9 100644
> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> @@ -180,10 +180,8 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>  	pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
>  
>  	res = rtw_alloc_hwxmits(padapter);
> -	if (res) {
> -		res = _FAIL;
> +	if (res == _FAIL)
>  		goto exit;
> -	}
>  
>  	rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
>  
> @@ -1510,7 +1508,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
>  
>  	pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
>  	if (!pxmitpriv->hwxmits)
> -		return -ENOMEM;
> +		return _FAIL;

No, please let's fix up the callers to properly detect normal kernel
error values and get rid of all of the crazy _FAIL and _SUCCESS values
in this driver.

>  
>  	hwxmits = pxmitpriv->hwxmits;
>  
> @@ -1528,7 +1526,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
>  	} else {
>  	}
>  
> -	return 0;
> +	return _SUCCESS;

Same here, fix up the callers if they are checking this incorrectly.

thanks,

greg k-h

Re: [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail

[Hans de Goede]

Hi Kate,

Good catch!

On 6/20/22 10:54, Kate Hsuan wrote:
> Since _SUCCESS (1) and _FAIL (0) are used to indicate the status of the
> functions. The previous commit 8ae7bf782eacad803f752c83a183393b0a67127b

This is the commit hash from one of the stable series backports, you
should always use the commit hash from Linus' master branch which is
f94b47c6bde6 and the format for referencing commits in a commit-message is:

commit <12 char hash> ("commit subject")

so in this case this should have been:

commit f94b47c6bde6 ("staging: r8188eu: add check for kzalloc")

Note that checkpatch.pl would have complained about the wrong format
(but not the wrong hash)

> fixed and prevented dereferencing a NULL pointer through checking the
> return pointer. The NULL pointer check work properly but the return
> values (-ENOMEM on fail and 0 on success). This work fixed the return
> values to make sure the caller function will return the correct status.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2097526
> Signed-off-by: Kate Hsuan <hpa@redhat.com>

This should have a fixes tag:

Fixes: f94b47c6bde6 ("staging: r8188eu: add check for kzalloc")

But while looking up the torvalds/master branch hash I noticed
someone alreayd beat you to it. Linus' master already has
a fix for this:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/staging/r8188eu?id=5b7419ae1d208cab1e2826d473d8dab045aa75c7

So this patch can be dropped since it is a duplicate.

Regards,

hans



> ---
>  drivers/staging/r8188eu/core/rtw_xmit.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> index f4e9f6102539..2f8720db21d9 100644
> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> @@ -180,10 +180,8 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>  	pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
>  
>  	res = rtw_alloc_hwxmits(padapter);
> -	if (res) {
> -		res = _FAIL;
> +	if (res == _FAIL)
>  		goto exit;
> -	}
>  
>  	rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
>  
> @@ -1510,7 +1508,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
>  
>  	pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
>  	if (!pxmitpriv->hwxmits)
> -		return -ENOMEM;
> +		return _FAIL;
>  
>  	hwxmits = pxmitpriv->hwxmits;
>  
> @@ -1528,7 +1526,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
>  	} else {
>  	}
>  
> -	return 0;
> +	return _SUCCESS;
>  }
>  
>  void rtw_free_hwxmits(struct adapter *padapter)

Re: [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail

[Kate Hsuan]

On Mon, Jun 20, 2022 at 5:01 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 20, 2022 at 04:54:13PM +0800, Kate Hsuan wrote:
> > Since _SUCCESS (1) and _FAIL (0) are used to indicate the status of the
> > functions. The previous commit 8ae7bf782eacad803f752c83a183393b0a67127b
>
> In the future, please reference commit ids as the documentation asks, as
> it's a much nicer way to understand things.  Also this commit id is not
> in Linus's tree or any tree that I can see, where did it come from?

Sorry about this. I'll improve this in my v2 patch.

>
> > fixed and prevented dereferencing a NULL pointer through checking the
> > return pointer. The NULL pointer check work properly but the return
> > values (-ENOMEM on fail and 0 on success). This work fixed the return
> > values to make sure the caller function will return the correct status.
> >
> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2097526
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> > ---
> >  drivers/staging/r8188eu/core/rtw_xmit.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> > index f4e9f6102539..2f8720db21d9 100644
> > --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> > +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> > @@ -180,10 +180,8 @@ s32      _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> >       pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
> >
> >       res = rtw_alloc_hwxmits(padapter);
> > -     if (res) {
> > -             res = _FAIL;
> > +     if (res == _FAIL)
> >               goto exit;
> > -     }
> >
> >       rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
> >
> > @@ -1510,7 +1508,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
> >
> >       pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
> >       if (!pxmitpriv->hwxmits)
> > -             return -ENOMEM;
> > +             return _FAIL;
>
> No, please let's fix up the callers to properly detect normal kernel
> error values and get rid of all of the crazy _FAIL and _SUCCESS values
> in this driver.

Okay.

>
> >
> >       hwxmits = pxmitpriv->hwxmits;
> >
> > @@ -1528,7 +1526,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
> >       } else {
> >       }
> >
> > -     return 0;
> > +     return _SUCCESS;
>
> Same here, fix up the callers if they are checking this incorrectly.
>
> thanks,
>
> greg k-h
>

Thank you :)

-- 
BR,
Kate

Re: [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail

[Kate Hsuan]

On Mon, Jun 20, 2022 at 5:02 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Kate,
>
> Good catch!
>
> On 6/20/22 10:54, Kate Hsuan wrote:
> > Since _SUCCESS (1) and _FAIL (0) are used to indicate the status of the
> > functions. The previous commit 8ae7bf782eacad803f752c83a183393b0a67127b
>
> This is the commit hash from one of the stable series backports, you
> should always use the commit hash from Linus' master branch which is
> f94b47c6bde6 and the format for referencing commits in a commit-message is:
>
> commit <12 char hash> ("commit subject")
>
> so in this case this should have been:
>
> commit f94b47c6bde6 ("staging: r8188eu: add check for kzalloc")
>
> Note that checkpatch.pl would have complained about the wrong format
> (but not the wrong hash)

Thanks for this. I'll improve my commit message.

>
> > fixed and prevented dereferencing a NULL pointer through checking the
> > return pointer. The NULL pointer check work properly but the return
> > values (-ENOMEM on fail and 0 on success). This work fixed the return
> > values to make sure the caller function will return the correct status.
> >
> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2097526
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
>
> This should have a fixes tag:
>
> Fixes: f94b47c6bde6 ("staging: r8188eu: add check for kzalloc")
>
> But while looking up the torvalds/master branch hash I noticed
> someone alreayd beat you to it. Linus' master already has
> a fix for this:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/staging/r8188eu?id=5b7419ae1d208cab1e2826d473d8dab045aa75c7
>
> So this patch can be dropped since it is a duplicate.

Ah~ I have to check before starting to fix it.

Thank you :)

>
> Regards,
>
> hans
>
>
>
> > ---
> >  drivers/staging/r8188eu/core/rtw_xmit.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> > index f4e9f6102539..2f8720db21d9 100644
> > --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> > +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> > @@ -180,10 +180,8 @@ s32      _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> >       pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
> >
> >       res = rtw_alloc_hwxmits(padapter);
> > -     if (res) {
> > -             res = _FAIL;
> > +     if (res == _FAIL)
> >               goto exit;
> > -     }
> >
> >       rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
> >
> > @@ -1510,7 +1508,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
> >
> >       pxmitpriv->hwxmits = kzalloc(sizeof(struct hw_xmit) * pxmitpriv->hwxmit_entry, GFP_KERNEL);
> >       if (!pxmitpriv->hwxmits)
> > -             return -ENOMEM;
> > +             return _FAIL;
> >
> >       hwxmits = pxmitpriv->hwxmits;
> >
> > @@ -1528,7 +1526,7 @@ int rtw_alloc_hwxmits(struct adapter *padapter)
> >       } else {
> >       }
> >
> > -     return 0;
> > +     return _SUCCESS;
> >  }
> >
> >  void rtw_free_hwxmits(struct adapter *padapter)
>


-- 
BR,
Kate

Re: [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail

[Larry Finger]

On 6/20/22 03:54, Kate Hsuan wrote:
> Since _SUCCESS (1) and _FAIL (0) are used to indicate the status of the
> functions. The previous commit 8ae7bf782eacad803f752c83a183393b0a67127b
> fixed and prevented dereferencing a NULL pointer through checking the
> return pointer. The NULL pointer check work properly but the return
> values (-ENOMEM on fail and 0 on success). This work fixed the return
> values to make sure the caller function will return the correct status.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=2097526
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>   drivers/staging/r8188eu/core/rtw_xmit.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_xmit.c b/drivers/staging/r8188eu/core/rtw_xmit.c
> index f4e9f6102539..2f8720db21d9 100644
> --- a/drivers/staging/r8188eu/core/rtw_xmit.c
> +++ b/drivers/staging/r8188eu/core/rtw_xmit.c
> @@ -180,10 +180,8 @@ s32	_rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
>   	pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;
>   
>   	res = rtw_alloc_hwxmits(padapter);
> -	if (res) {
> -		res = _FAIL;
> +	if (res == _FAIL)
>   		goto exit;
> -	}
>   
>   	rtw_init_hwxmits(pxmitpriv->hwxmits, pxmitpriv->hwxmit_entry);
>   

This problem was fixed in mid May with commit 5b7419ae1d20 ("staging: r8188eu: 
fix rtw_alloc_hwxmits error detection for now"). The fix was

@@ -178,8 +178,7 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct 
adapter *padapter)

         pxmitpriv->free_xmit_extbuf_cnt = num_xmit_extbuf;

-       res = rtw_alloc_hwxmits(padapter);
-       if (res) {
+       if (rtw_alloc_hwxmits(padapter)) {
                 res = _FAIL;
                 goto exit;
         }

The "for now" part is that Phillip plans to get rid of _FAIL and _SUCCESS, and 
replace the logic with a normal 1 for fail, etc.; however, this will be a major 
change that must be done carefully.

In any case NACK for this patch.

Larry

Re: [PATCH v1] staging: r8188eu: an incorrect return value made the function always return fail

[Phillip Potter]

On Mon, Jun 20, 2022 at 09:05:17AM -0500, Larry Finger wrote:
> 
> The "for now" part is that Phillip plans to get rid of _FAIL and _SUCCESS,
> and replace the logic with a normal 1 for fail, etc.; however, this will be
> a major change that must be done carefully.
> 
> In any case NACK for this patch.
> 
> Larry
> 
> 
>

Hi all,

I would be happy to work on purging _FAIL and _SUCCESS from the driver
by all means - definitely a worthwhile goal. I know Pavel said he
might take a look too though - copying him in therefore to see if we can
coordinate an approach - happy to do as much or as little as desired.

Regards,
Phil