[PATCH 1/3] staging: rtl8712: change the type of _r8712_init_recv_priv()

[PATCH 1/3] staging: rtl8712: change the type of _r8712_init_recv_priv()

[xkernel.wang]

From: Xiaoke Wang <xkernel.wang@foxmail.com>

There is a memory allocation in _r8712_init_recv_priv(). Since the
original type of this function is void, now it is changed to int to
make the error of allocation failures propagate to its caller easily.

Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
---
 drivers/staging/rtl8712/recv_osdep.h   | 2 +-
 drivers/staging/rtl8712/rtl871x_recv.c | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8712/recv_osdep.h b/drivers/staging/rtl8712/recv_osdep.h
index d8c1fa7..f5b97c5 100644
--- a/drivers/staging/rtl8712/recv_osdep.h
+++ b/drivers/staging/rtl8712/recv_osdep.h
@@ -18,7 +18,7 @@
 #include "drv_types.h"
 #include <linux/skbuff.h>
 
-void _r8712_init_recv_priv(struct recv_priv *precvpriv,
+int _r8712_init_recv_priv(struct recv_priv *precvpriv,
 			   struct _adapter *padapter);
 void _r8712_free_recv_priv(struct recv_priv *precvpriv);
 void r8712_recv_entry(union recv_frame *precv_frame);
diff --git a/drivers/staging/rtl8712/rtl871x_recv.c b/drivers/staging/rtl8712/rtl871x_recv.c
index c23f6b3..dd8cb07 100644
--- a/drivers/staging/rtl8712/rtl871x_recv.c
+++ b/drivers/staging/rtl8712/rtl871x_recv.c
@@ -44,7 +44,7 @@ void _r8712_init_sta_recv_priv(struct sta_recv_priv *psta_recvpriv)
 	_init_queue(&psta_recvpriv->defrag_q);
 }
 
-void _r8712_init_recv_priv(struct recv_priv *precvpriv,
+int _r8712_init_recv_priv(struct recv_priv *precvpriv,
 			   struct _adapter *padapter)
 {
 	sint i;
@@ -60,7 +60,7 @@ void _r8712_init_recv_priv(struct recv_priv *precvpriv,
 				sizeof(union recv_frame) + RXFRAME_ALIGN_SZ,
 				GFP_ATOMIC);
 	if (!precvpriv->pallocated_frame_buf)
-		return;
+		return -ENOMEM;
 	kmemleak_not_leak(precvpriv->pallocated_frame_buf);
 	precvpriv->precv_frame_buf = precvpriv->pallocated_frame_buf +
 				    RXFRAME_ALIGN_SZ -
@@ -77,6 +77,7 @@ void _r8712_init_recv_priv(struct recv_priv *precvpriv,
 	}
 	precvpriv->rx_pending_cnt = 1;
 	r8712_init_recv_priv(precvpriv, padapter);
+	return 0;
 }
 
 void _r8712_free_recv_priv(struct recv_priv *precvpriv)
-- 

[PATCH 2/3] staging: rtl8712: add two validation check in r8712_init_drv_sw()

[xkernel.wang]

From: Xiaoke Wang <xkernel.wang@foxmail.com>

_r8712_init_xmit_priv() or _r8712_init_recv_priv() returns -ENOMEM
when some allocations inside it failed.
So it is better to check the return status of them.

Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
---
 drivers/staging/rtl8712/os_intfs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8712/os_intfs.c b/drivers/staging/rtl8712/os_intfs.c
index 9502f6a..163baaa 100644
--- a/drivers/staging/rtl8712/os_intfs.c
+++ b/drivers/staging/rtl8712/os_intfs.c
@@ -308,8 +308,12 @@ int r8712_init_drv_sw(struct _adapter *padapter)
 	ret = r8712_init_mlme_priv(padapter);
 	if (ret)
 		return ret;
-	_r8712_init_xmit_priv(&padapter->xmitpriv, padapter);
-	_r8712_init_recv_priv(&padapter->recvpriv, padapter);
+	ret = _r8712_init_xmit_priv(&padapter->xmitpriv, padapter);
+	if (ret)
+		return ret;
+	ret = _r8712_init_recv_priv(&padapter->recvpriv, padapter);
+	if (ret)
+		return ret;
 	memset((unsigned char *)&padapter->securitypriv, 0,
 	       sizeof(struct security_priv));
 	timer_setup(&padapter->securitypriv.tkip_timer,
-- 

Re: [PATCH 2/3] staging: rtl8712: add two validation check in r8712_init_drv_sw()

[Greg KH]

On Fri, Apr 01, 2022 at 01:07:45AM +0800, xkernel.wang@foxmail.com wrote:
> From: Xiaoke Wang <xkernel.wang@foxmail.com>
> 
> _r8712_init_xmit_priv() or _r8712_init_recv_priv() returns -ENOMEM
> when some allocations inside it failed.
> So it is better to check the return status of them.
> 
> Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
> ---
>  drivers/staging/rtl8712/os_intfs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8712/os_intfs.c b/drivers/staging/rtl8712/os_intfs.c
> index 9502f6a..163baaa 100644
> --- a/drivers/staging/rtl8712/os_intfs.c
> +++ b/drivers/staging/rtl8712/os_intfs.c
> @@ -308,8 +308,12 @@ int r8712_init_drv_sw(struct _adapter *padapter)
>  	ret = r8712_init_mlme_priv(padapter);
>  	if (ret)
>  		return ret;
> -	_r8712_init_xmit_priv(&padapter->xmitpriv, padapter);
> -	_r8712_init_recv_priv(&padapter->recvpriv, padapter);
> +	ret = _r8712_init_xmit_priv(&padapter->xmitpriv, padapter);
> +	if (ret)
> +		return ret;
> +	ret = _r8712_init_recv_priv(&padapter->recvpriv, padapter);
> +	if (ret)
> +		return ret;

You just leaked memory :(

please please please test these types of "fix up error handling"
changes, as there are lots and lots of ways to get these wrong.

If you can not test them, provide some sort of proof that the change is
correct please.

thanks,

greg k-h

Re: [PATCH 2/3] staging: rtl8712: add two validation check in r8712_init_drv_sw()

[xkernel.wang]

On Monday, April 4, 2022 10:30 PM +0800, gregkh@linuxfoundation.org wrote:
> You just leaked memory :(
>
> please please please test these types of "fix up error handling"
> changes, as there are lots and lots of ways to get these wrong.
>

It seems that you missed the third email in this series.

In fact, I did not ignore the memory leak problem. But considering that there are other paths which can also lead to memory leak, I separated another patch to deal with them specifically.

In the next version, I will adjust the sequence of the patches and add some descriptions to explicitly show the relations.

--
Regards,
Xiaoke Wang

[PATCH 3/3] staging: rtl8712: fix potential memory leak in r8712_init_drv_sw()

[xkernel.wang]

From: Xiaoke Wang <xkernel.wang@foxmail.com>

In r8712_init_drv_sw(), all the error paths do not properly release the
resources allocated by its callees.
This patch is to free them.

Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
---
 drivers/staging/rtl8712/os_intfs.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8712/os_intfs.c b/drivers/staging/rtl8712/os_intfs.c
index 163baaa..28b1684 100644
--- a/drivers/staging/rtl8712/os_intfs.c
+++ b/drivers/staging/rtl8712/os_intfs.c
@@ -304,23 +304,23 @@ int r8712_init_drv_sw(struct _adapter *padapter)
 	padapter->cmdpriv.padapter = padapter;
 	ret = r8712_init_evt_priv(&padapter->evtpriv);
 	if (ret)
-		return ret;
+		goto free_cmd_priv;
 	ret = r8712_init_mlme_priv(padapter);
 	if (ret)
-		return ret;
+		goto free_evt_priv;
 	ret = _r8712_init_xmit_priv(&padapter->xmitpriv, padapter);
 	if (ret)
-		return ret;
+		goto free_mlme_priv;
 	ret = _r8712_init_recv_priv(&padapter->recvpriv, padapter);
 	if (ret)
-		return ret;
+		goto free_xmit_priv;
 	memset((unsigned char *)&padapter->securitypriv, 0,
 	       sizeof(struct security_priv));
 	timer_setup(&padapter->securitypriv.tkip_timer,
 		    r8712_use_tkipkey_handler, 0);
 	ret = _r8712_init_sta_priv(&padapter->stapriv);
 	if (ret)
-		return ret;
+		goto free_recv_priv;
 	padapter->stapriv.padapter = padapter;
 	r8712_init_bcmc_stainfo(padapter);
 	r8712_init_pwrctrl_priv(padapter);
@@ -328,6 +328,18 @@ int r8712_init_drv_sw(struct _adapter *padapter)
 	init_default_value(padapter);
 	r8712_InitSwLeds(padapter);
 	return ret;
+
+free_recv_priv:
+	_r8712_free_recv_priv(&padapter->recvpriv);
+free_xmit_priv:
+	_free_xmit_priv(&padapter->xmitpriv);
+free_mlme_priv:
+	r8712_free_mlme_priv(&padapter->mlmepriv);
+free_evt_priv:
+	r8712_free_evt_priv(&padapter->evtpriv);
+free_cmd_priv:
+	r8712_free_cmd_priv(&padapter->cmdpriv);
+	return ret;
 }
 
 void r8712_free_drv_sw(struct _adapter *padapter)
-- 

Re: [PATCH 3/3] staging: rtl8712: fix potential memory leak in r8712_init_drv_sw()

[Dan Carpenter]

On Fri, Apr 01, 2022 at 01:08:19AM +0800, xkernel.wang@foxmail.com wrote:
>  	ret = _r8712_init_sta_priv(&padapter->stapriv);
>  	if (ret)
> -		return ret;
> +		goto free_recv_priv;
>  	padapter->stapriv.padapter = padapter;
>  	r8712_init_bcmc_stainfo(padapter);
>  	r8712_init_pwrctrl_priv(padapter);
> @@ -328,6 +328,18 @@ int r8712_init_drv_sw(struct _adapter *padapter)
>  	init_default_value(padapter);
>  	r8712_InitSwLeds(padapter);
>  	return ret;

Not related to the patch.  (You didn't introduce this, so it's not
something you need to fix.)  But this "return ret;" would be better if
it were "return 0;"

regards,
dan carpenter


> +
> +free_recv_priv:
> +	_r8712_free_recv_priv(&padapter->recvpriv);
> +free_xmit_priv:
> +	_free_xmit_priv(&padapter->xmitpriv);
> +free_mlme_priv:
> +	r8712_free_mlme_priv(&padapter->mlmepriv);
> +free_evt_priv:
> +	r8712_free_evt_priv(&padapter->evtpriv);
> +free_cmd_priv:
> +	r8712_free_cmd_priv(&padapter->cmdpriv);
> +	return ret;
>  }

Re: [PATCH 1/3] staging: rtl8712: change the type of _r8712_init_recv_priv()

[Greg KH]

On Fri, Apr 01, 2022 at 01:06:35AM +0800, xkernel.wang@foxmail.com wrote:
> From: Xiaoke Wang <xkernel.wang@foxmail.com>
> 
> There is a memory allocation in _r8712_init_recv_priv(). Since the
> original type of this function is void, now it is changed to int to
> make the error of allocation failures propagate to its caller easily.
> 
> Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
> ---
>  drivers/staging/rtl8712/recv_osdep.h   | 2 +-
>  drivers/staging/rtl8712/rtl871x_recv.c | 5 +++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/rtl8712/recv_osdep.h b/drivers/staging/rtl8712/recv_osdep.h
> index d8c1fa7..f5b97c5 100644
> --- a/drivers/staging/rtl8712/recv_osdep.h
> +++ b/drivers/staging/rtl8712/recv_osdep.h
> @@ -18,7 +18,7 @@
>  #include "drv_types.h"
>  #include <linux/skbuff.h>
>  
> -void _r8712_init_recv_priv(struct recv_priv *precvpriv,
> +int _r8712_init_recv_priv(struct recv_priv *precvpriv,
>  			   struct _adapter *padapter);
>  void _r8712_free_recv_priv(struct recv_priv *precvpriv);
>  void r8712_recv_entry(union recv_frame *precv_frame);
> diff --git a/drivers/staging/rtl8712/rtl871x_recv.c b/drivers/staging/rtl8712/rtl871x_recv.c
> index c23f6b3..dd8cb07 100644
> --- a/drivers/staging/rtl8712/rtl871x_recv.c
> +++ b/drivers/staging/rtl8712/rtl871x_recv.c
> @@ -44,7 +44,7 @@ void _r8712_init_sta_recv_priv(struct sta_recv_priv *psta_recvpriv)
>  	_init_queue(&psta_recvpriv->defrag_q);
>  }
>  
> -void _r8712_init_recv_priv(struct recv_priv *precvpriv,
> +int _r8712_init_recv_priv(struct recv_priv *precvpriv,
>  			   struct _adapter *padapter)
>  {
>  	sint i;
> @@ -60,7 +60,7 @@ void _r8712_init_recv_priv(struct recv_priv *precvpriv,
>  				sizeof(union recv_frame) + RXFRAME_ALIGN_SZ,
>  				GFP_ATOMIC);
>  	if (!precvpriv->pallocated_frame_buf)
> -		return;
> +		return -ENOMEM;
>  	kmemleak_not_leak(precvpriv->pallocated_frame_buf);

Why are you still telling kmemleak that this was not a leak if you
properly handle it?

thanks,

greg k-h