[PATCH 0/3] staging: r8188eu: fix some null checks

[PATCH 0/3] staging: r8188eu: fix some null checks

[Vihas Makwana]

There are some NULL checks which doesn't make sense. 
So either we should remove them or fix them appropriately.
This patchset fixes them.
Tested on Comfast CF-WU810N RTL8188EUS wireless adapter.

Vihas Makwana (3):
  staging: r8188eu: fix null check in rtw_free_recvframe
  staging: r8188eu: fix null check in _rtw_enqueue_recvframe
  staging: r8188eu: fix null check in _rtw_free_mlme_priv

 drivers/staging/r8188eu/core/rtw_mlme.c | 8 +++-----
 drivers/staging/r8188eu/core/rtw_recv.c | 5 +++--
 2 files changed, 6 insertions(+), 7 deletions(-)

-- 
2.30.2

[PATCH 1/3] staging: r8188eu: fix null check in rtw_free_recvframe

[Vihas Makwana]

There's a NULL check on padapter in rtw_recv.c:170 which makes no sense as
rtw_recv.c:152 dereferences it unconditionally and it would have already
crashed at this point.
Fix this by moving the dereference line inside the check.

Signed-off-by: Vihas Makwana <makvihas@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_recv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
index c1005ddaa..4cf9b4b8f 100644
--- a/drivers/staging/r8188eu/core/rtw_recv.c
+++ b/drivers/staging/r8188eu/core/rtw_recv.c
@@ -154,7 +154,6 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
 	if (!precvframe)
 		return _FAIL;
 	padapter = precvframe->adapter;
-	precvpriv = &padapter->recvpriv;
 	if (precvframe->pkt) {
 		dev_kfree_skb_any(precvframe->pkt);/* free skb by driver */
 		precvframe->pkt = NULL;
@@ -169,6 +168,7 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
 	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
 
 	if (padapter) {
+		precvpriv = &padapter->recvpriv;
 		if (pfree_recv_queue == &precvpriv->free_recv_queue)
 				precvpriv->free_recvframe_cnt++;
 	}
-- 
2.30.2

Re: [PATCH 1/3] staging: r8188eu: fix null check in rtw_free_recvframe

[Pavel Skripkin]

Hi Vihas,

On 4/24/22 19:31, Vihas Makwana wrote:
> There's a NULL check on padapter in rtw_recv.c:170 which makes no sense as
> rtw_recv.c:152 dereferences it unconditionally and it would have already
> crashed at this point.
> Fix this by moving the dereference line inside the check.
> 
> Signed-off-by: Vihas Makwana <makvihas@gmail.com>
> ---
>   drivers/staging/r8188eu/core/rtw_recv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
> index c1005ddaa..4cf9b4b8f 100644
> --- a/drivers/staging/r8188eu/core/rtw_recv.c
> +++ b/drivers/staging/r8188eu/core/rtw_recv.c
> @@ -154,7 +154,6 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
>   	if (!precvframe)
>   		return _FAIL;
>   	padapter = precvframe->adapter;
> -	precvpriv = &padapter->recvpriv;

This expression just an addr calculation, so it does not lead to panic 
in case of padapter being NULL

>   	if (precvframe->pkt) {
>   		dev_kfree_skb_any(precvframe->pkt);/* free skb by driver */
>   		precvframe->pkt = NULL;
> @@ -169,6 +168,7 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
>   	list_add_tail(&precvframe->list, get_list_head(pfree_recv_queue));
>   
>   	if (padapter) {
> +		precvpriv = &padapter->recvpriv;
>   		if (pfree_recv_queue == &precvpriv->free_recv_queue)
>   				precvpriv->free_recvframe_cnt++;
>   	}




With regards,
Pavel Skripkin

[PATCH 2/3] staging: r8188eu: fix null check in _rtw_enqueue_recvframe

[Vihas Makwana]

There's a NULL check on padapter in rtw_recv.c:189 which makes no sense as
rtw_recv.c:184 dereferences it unconditionally and it would have already
crashed at this point.
Fix this by moving the dereference line inside the check.

Signed-off-by: Vihas Makwana <makvihas@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_recv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
index 4cf9b4b8f..7d306a3c6 100644
--- a/drivers/staging/r8188eu/core/rtw_recv.c
+++ b/drivers/staging/r8188eu/core/rtw_recv.c
@@ -181,12 +181,13 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
 int _rtw_enqueue_recvframe(struct recv_frame *precvframe, struct __queue *queue)
 {
 	struct adapter *padapter = precvframe->adapter;
-	struct recv_priv *precvpriv = &padapter->recvpriv;
+	struct recv_priv *precvpriv;
 
 	list_del_init(&precvframe->list);
 	list_add_tail(&precvframe->list, get_list_head(queue));
 
 	if (padapter) {
+		precvpriv = &padapter->recvpriv;
 		if (queue == &precvpriv->free_recv_queue)
 			precvpriv->free_recvframe_cnt++;
 	}
-- 
2.30.2

Re: [PATCH 2/3] staging: r8188eu: fix null check in _rtw_enqueue_recvframe

[Pavel Skripkin]

Hi Vihas,

On 4/24/22 19:31, Vihas Makwana wrote:
> There's a NULL check on padapter in rtw_recv.c:189 which makes no sense as
> rtw_recv.c:184 dereferences it unconditionally and it would have already
> crashed at this point.
> Fix this by moving the dereference line inside the check.
> 
> Signed-off-by: Vihas Makwana <makvihas@gmail.com>
> ---
>   drivers/staging/r8188eu/core/rtw_recv.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
> index 4cf9b4b8f..7d306a3c6 100644
> --- a/drivers/staging/r8188eu/core/rtw_recv.c
> +++ b/drivers/staging/r8188eu/core/rtw_recv.c
> @@ -181,12 +181,13 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
>   int _rtw_enqueue_recvframe(struct recv_frame *precvframe, struct __queue *queue)
>   {
>   	struct adapter *padapter = precvframe->adapter;
> -	struct recv_priv *precvpriv = &padapter->recvpriv;
> +	struct recv_priv *precvpriv;

Actually, `&padapter->recvpriv` is not a de-reference, it's just address 
calculation, so in case of padapder being NULL precvpriv will contain 
offsetof(struct adapter, recvpriv).

>   
>   	list_del_init(&precvframe->list);
>   	list_add_tail(&precvframe->list, get_list_head(queue));
>   
>   	if (padapter) {
> +		precvpriv = &padapter->recvpriv;
>   		if (queue == &precvpriv->free_recv_queue)
>   			precvpriv->free_recvframe_cnt++;
>   	}




With regards,
Pavel Skripkin

Re: [PATCH 2/3] staging: r8188eu: fix null check in _rtw_enqueue_recvframe

[Vihas Makwana]

On Mon, Apr 25, 2022 at 12:23 AM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> Hi Vihas,
>
> On 4/24/22 19:31, Vihas Makwana wrote:
> > There's a NULL check on padapter in rtw_recv.c:189 which makes no sense as
> > rtw_recv.c:184 dereferences it unconditionally and it would have already
> > crashed at this point.
> > Fix this by moving the dereference line inside the check.
> >
> > Signed-off-by: Vihas Makwana <makvihas@gmail.com>
> > ---
> >   drivers/staging/r8188eu/core/rtw_recv.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
> > index 4cf9b4b8f..7d306a3c6 100644
> > --- a/drivers/staging/r8188eu/core/rtw_recv.c
> > +++ b/drivers/staging/r8188eu/core/rtw_recv.c
> > @@ -181,12 +181,13 @@ int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv
> >   int _rtw_enqueue_recvframe(struct recv_frame *precvframe, struct __queue *queue)
> >   {
> >       struct adapter *padapter = precvframe->adapter;
> > -     struct recv_priv *precvpriv = &padapter->recvpriv;
> > +     struct recv_priv *precvpriv;
>
> Actually, `&padapter->recvpriv` is not a de-reference, it's just address
> calculation, so in case of padapder being NULL precvpriv will contain
> offsetof(struct adapter, recvpriv).
>
Oh, I see.
> >
> >       list_del_init(&precvframe->list);
> >       list_add_tail(&precvframe->list, get_list_head(queue));
> >
> >       if (padapter) {
> > +             precvpriv = &padapter->recvpriv;
> >               if (queue == &precvpriv->free_recv_queue)
> >                       precvpriv->free_recvframe_cnt++;
> >       }
>
>
>
>
> With regards,
> Pavel Skripkin



-- 
Thanks,
Vihas

[PATCH 3/3] staging: r8188eu: fix null check in _rtw_free_mlme_priv

[Vihas Makwana]

There's a NULL check on pmlmepriv in rtw_mlme.c:112 which makes no sense
as rtw_free_mlme_priv_ie_data() dereferences it unconditionally and it
would have already crashed at this point. 
Fix this by moving rtw_free_mlme_priv_ie_data() inside the check.

Signed-off-by: Vihas Makwana <makvihas@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_mlme.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/r8188eu/core/rtw_mlme.c
index 081c02417..87c754462 100644
--- a/drivers/staging/r8188eu/core/rtw_mlme.c
+++ b/drivers/staging/r8188eu/core/rtw_mlme.c
@@ -109,12 +109,10 @@ void rtw_free_mlme_priv_ie_data(struct mlme_priv *pmlmepriv)
 
 void _rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
 {
-
-	rtw_free_mlme_priv_ie_data(pmlmepriv);
-
-	if (pmlmepriv)
+	if (pmlmepriv) {
+		rtw_free_mlme_priv_ie_data(pmlmepriv);
 		vfree(pmlmepriv->free_bss_buf);
-
+	}
 }
 
 struct	wlan_network *_rtw_alloc_network(struct	mlme_priv *pmlmepriv)/* _queue *free_queue) */
-- 
2.30.2

Re: [PATCH 3/3] staging: r8188eu: fix null check in _rtw_free_mlme_priv

[Pavel Skripkin]

Hi Vihas,

On 4/24/22 19:31, Vihas Makwana wrote:
> There's a NULL check on pmlmepriv in rtw_mlme.c:112 which makes no sense
> as rtw_free_mlme_priv_ie_data() dereferences it unconditionally and it
> would have already crashed at this point.
> Fix this by moving rtw_free_mlme_priv_ie_data() inside the check.
> 
> Signed-off-by: Vihas Makwana <makvihas@gmail.com>

That's good catch, but looks like the check is just redundant

This function is called only from it's wrapper called 
rtw_free_mlme_priv() and rtw_free_mlme_priv() is called from 2 places:

4 drivers/staging/r8188eu/os_dep/os_intfs.c|531 col 2| 
rtw_free_mlme_priv(&padapter->mlmepriv);
5 drivers/staging/r8188eu/os_dep/os_intfs.c|579 col 2| 
rtw_free_mlme_priv(&padapter->mlmepriv);

_Very_ unlikely that `&padapter->mlmepriv` expression will become NULL.


> ---
>   drivers/staging/r8188eu/core/rtw_mlme.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/r8188eu/core/rtw_mlme.c
> index 081c02417..87c754462 100644
> --- a/drivers/staging/r8188eu/core/rtw_mlme.c
> +++ b/drivers/staging/r8188eu/core/rtw_mlme.c
> @@ -109,12 +109,10 @@ void rtw_free_mlme_priv_ie_data(struct mlme_priv *pmlmepriv)
>   
>   void _rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
>   {
> -
> -	rtw_free_mlme_priv_ie_data(pmlmepriv);
> -
> -	if (pmlmepriv)
> +	if (pmlmepriv) {
> +		rtw_free_mlme_priv_ie_data(pmlmepriv);
>   		vfree(pmlmepriv->free_bss_buf);
> -
> +	}
>   }
>   
>   struct	wlan_network *_rtw_alloc_network(struct	mlme_priv *pmlmepriv)/* _queue *free_queue) */




With regards,
Pavel Skripkin

Re: [PATCH 3/3] staging: r8188eu: fix null check in _rtw_free_mlme_priv

[Vihas Makwana]

On Mon, Apr 25, 2022 at 12:30 AM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> Hi Vihas,
>
> On 4/24/22 19:31, Vihas Makwana wrote:
> > There's a NULL check on pmlmepriv in rtw_mlme.c:112 which makes no sense
> > as rtw_free_mlme_priv_ie_data() dereferences it unconditionally and it
> > would have already crashed at this point.
> > Fix this by moving rtw_free_mlme_priv_ie_data() inside the check.
> >
> > Signed-off-by: Vihas Makwana <makvihas@gmail.com>
>
> That's good catch, but looks like the check is just redundant
>
> This function is called only from it's wrapper called
> rtw_free_mlme_priv() and rtw_free_mlme_priv() is called from 2 places:
>
> 4 drivers/staging/r8188eu/os_dep/os_intfs.c|531 col 2|
> rtw_free_mlme_priv(&padapter->mlmepriv);
> 5 drivers/staging/r8188eu/os_dep/os_intfs.c|579 col 2|
> rtw_free_mlme_priv(&padapter->mlmepriv);
>
> _Very_ unlikely that `&padapter->mlmepriv` expression will become NULL.
>
So I guess either we should remove the check or mark it with the
`unlikely()` macro.
>
> > ---
> >   drivers/staging/r8188eu/core/rtw_mlme.c | 8 +++-----
> >   1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/r8188eu/core/rtw_mlme.c
> > index 081c02417..87c754462 100644
> > --- a/drivers/staging/r8188eu/core/rtw_mlme.c
> > +++ b/drivers/staging/r8188eu/core/rtw_mlme.c
> > @@ -109,12 +109,10 @@ void rtw_free_mlme_priv_ie_data(struct mlme_priv *pmlmepriv)
> >
> >   void _rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
> >   {
> > -
> > -     rtw_free_mlme_priv_ie_data(pmlmepriv);
> > -
> > -     if (pmlmepriv)
> > +     if (pmlmepriv) {
> > +             rtw_free_mlme_priv_ie_data(pmlmepriv);
> >               vfree(pmlmepriv->free_bss_buf);
> > -
> > +     }
> >   }
> >
> >   struct      wlan_network *_rtw_alloc_network(struct mlme_priv *pmlmepriv)/* _queue *free_queue) */
>
>
>
>
> With regards,
> Pavel Skripkin



-- 
Thanks,
Vihas

Re: [PATCH 3/3] staging: r8188eu: fix null check in _rtw_free_mlme_priv

[Dan Carpenter]

On Mon, Apr 25, 2022 at 11:25:32PM +0530, Vihas Makwana wrote:
> On Mon, Apr 25, 2022 at 12:30 AM Pavel Skripkin <paskripkin@gmail.com> wrote:
> >
> > Hi Vihas,
> >
> > On 4/24/22 19:31, Vihas Makwana wrote:
> > > There's a NULL check on pmlmepriv in rtw_mlme.c:112 which makes no sense
> > > as rtw_free_mlme_priv_ie_data() dereferences it unconditionally and it
> > > would have already crashed at this point.
> > > Fix this by moving rtw_free_mlme_priv_ie_data() inside the check.
> > >
> > > Signed-off-by: Vihas Makwana <makvihas@gmail.com>
> >
> > That's good catch, but looks like the check is just redundant
> >
> > This function is called only from it's wrapper called
> > rtw_free_mlme_priv() and rtw_free_mlme_priv() is called from 2 places:
> >
> > 4 drivers/staging/r8188eu/os_dep/os_intfs.c|531 col 2|
> > rtw_free_mlme_priv(&padapter->mlmepriv);
> > 5 drivers/staging/r8188eu/os_dep/os_intfs.c|579 col 2|
> > rtw_free_mlme_priv(&padapter->mlmepriv);
> >
> > _Very_ unlikely that `&padapter->mlmepriv` expression will become NULL.
> >
> So I guess either we should remove the check or mark it with the
> `unlikely()` macro.

The likely/unlikely() macros are only for when you can prove it makes a
difference to benchmark.  They hurt readability, but they're important
for optimizing the fast path.

Just remove the check.

regards,
dan carpenter