staging: r8188eu: fix some null checks

PATCH In review Vihas Makwana 3 participants 10 messages Apr 24, 2022 – Apr 25, 2022 Staging

Share & tools

Vihas Makwana

author

2022-04-24 16:31 UTC #0 lore raw

cover staging: r8188eu: fix some null checks

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(-)

Signature · 1 line

2.30.2

Vihas Makwana

author

2022-04-24 16:31 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_recv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/r8188eu/core/rtw_recv.c +1 -1

index c1005ddaa..4cf9b4b8f 100644
@@ -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++;
 	}

Signature · 1 line

2.30.2

Pavel Skripkin

2022-04-24 19:01 UTC #2 lore raw

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

Hi Vihas,

Quoted 15 lines · 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 <[email protected]>
---
  drivers/staging/r8188eu/core/rtw_recv.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/r8188eu/core/rtw_recv.c +0 -0
index c1005ddaa..4cf9b4b8f 100644

drivers/staging/r8188eu/core/rtw_recv.c int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv · line 157

Quoted 4 lines

drivers/staging/r8188eu/core/rtw_recv.c +0 -1

@@ -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

drivers/staging/r8188eu/core/rtw_recv.c int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv · line 174

Quoted 11 lines

drivers/staging/r8188eu/core/rtw_recv.c +1 -0

  	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

Vihas Makwana

author

2022-04-24 16:31 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_recv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/staging/r8188eu/core/rtw_recv.c +2 -1

index 4cf9b4b8f..7d306a3c6 100644
@@ -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++;
 	}

Signature · 1 line

2.30.2

Pavel Skripkin

2022-04-24 18:53 UTC #4 lore raw

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

Hi Vihas,

Quoted 15 lines · 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 <[email protected]>
---
  drivers/staging/r8188eu/core/rtw_recv.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/staging/r8188eu/core/rtw_recv.c +0 -0
index 4cf9b4b8f..7d306a3c6 100644

drivers/staging/r8188eu/core/rtw_recv.c int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv · line 184

Quoted 5 lines

drivers/staging/r8188eu/core/rtw_recv.c +1 -1

@@ -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).

drivers/staging/r8188eu/core/rtw_recv.c int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv · line 193

Quoted 9 lines

drivers/staging/r8188eu/core/rtw_recv.c +1 -0

  	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

Vihas Makwana

author

2022-04-25 17:47 UTC #5 lore raw

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

Quoted 14 lines · Pavel Skripkin

Hi Vihas,
Quoted 10 lines · 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 <[email protected]>
---
  drivers/staging/r8188eu/core/rtw_recv.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

Quoted 15 lines

drivers/staging/r8188eu/core/rtw_recv.c int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv · line 184
Quoted 10 lines
drivers/staging/r8188eu/core/rtw_recv.c +1 -1
index 4cf9b4b8f..7d306a3c6 100644
@@ -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.

Quoted 15 lines

drivers/staging/r8188eu/core/rtw_recv.c int rtw_free_recvframe(struct recv_frame *precvframe, struct __queue *pfree_recv · line 193
Quoted 9 lines
drivers/staging/r8188eu/core/rtw_recv.c +1 -0
      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

Signature · 2 lines

Thanks,
Vihas

Vihas Makwana

author

2022-04-24 16:31 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_mlme.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/staging/r8188eu/core/rtw_mlme.c +3 -5

index 081c02417..87c754462 100644
@@ -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) */

Signature · 1 line

2.30.2

Pavel Skripkin

2022-04-24 19:00 UTC #7 lore raw

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

Hi Vihas,

Quoted 6 lines · 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 <[email protected]>

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.

Quoted 14 lines

---
  drivers/staging/r8188eu/core/rtw_mlme.c | 8 +++-----
  1 file changed, 3 insertions(+), 5 deletions(-)

drivers/staging/r8188eu/core/rtw_mlme.c +0 -2

index 081c02417..87c754462 100644
@@ -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);

drivers/staging/r8188eu/core/rtw_mlme.c void rtw_free_mlme_priv_ie_data(struct mlme_priv *pmlmepriv) · line 118

Quoted 10 lines

drivers/staging/r8188eu/core/rtw_mlme.c +3 -3

-
-	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

Vihas Makwana

author

2022-04-25 17:55 UTC #8 lore raw

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

Quoted 16 lines · Pavel Skripkin

Hi Vihas,
Quoted 6 lines · 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 <[email protected]>
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:

Quoted 7 lines

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.

Quoted 31 lines

Quoted 15 lines

---
  drivers/staging/r8188eu/core/rtw_mlme.c | 8 +++-----
  1 file changed, 3 insertions(+), 5 deletions(-)

drivers/staging/r8188eu/core/rtw_mlme.c +0 -2

index 081c02417..87c754462 100644
@@ -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);

drivers/staging/r8188eu/core/rtw_mlme.c void rtw_free_mlme_priv_ie_data(struct mlme_priv *pmlmepriv) · line 118

Quoted 10 lines

drivers/staging/r8188eu/core/rtw_mlme.c +3 -3

-
-     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

Signature · 2 lines

Thanks,
Vihas

Dan Carpenter

2022-04-25 18:48 UTC #9 lore raw

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

Quoted 22 lines · Vihas Makwana

Quoted 21 lines · Pavel Skripkin
Hi Vihas,
Quoted 6 lines · 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 <[email protected]>
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);

Quoted 4 lines

Quoted 2 lines
_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