Re: [PATCH] [staging][r8188eu]: memory leak in rtw_free_cmd_obj if command is (_Set_Drv_Extra)
On Tue, Mar 18, 2014 at 10:25:27AM -0400, Wang, Xiaoming wrote: > pcmd->parmbuf->pbuf has been allocated if command is > GEN_CMD_CODE(_Set_Drv_Extra), > and it enqueued by rtw_enqueue_cmd. rtw_cmd_thread dequeue pcmd by > rtw_dequeue_cmd. > The memory leak happened on this branch "if( _FAIL == > rtw_cmd_filter(pcmdpriv, pcmd) )" > which goto post_process directly against freeing pcmd->parmbuf->pbuf in > rtw_drvextra_cmd_hdl which is the cmd_hdl if command is (_Set_Drv_Extra). > This patch free pcmd->parmbuf->pbuf on the forgotten branch to avoid memory > leak. > > Signed-off-by: Zhang Dongxing > Signed-off-by: xiaoming wang > > diff --git a/drivers/staging/rtl8188eu/core/rtw_cmd.c > b/drivers/staging/rtl8188eu/core/rtw_cmd.c > index c0a0a52..1c7f505 100644 > --- a/drivers/staging/rtl8188eu/core/rtw_cmd.c > +++ b/drivers/staging/rtl8188eu/core/rtw_cmd.c > @@ -288,7 +288,7 @@ int rtw_cmd_thread(void *context) > void (*pcmd_callback)(struct adapter *dev, struct cmd_obj *pcmd); > struct adapter *padapter = (struct adapter *)context; > struct cmd_priv *pcmdpriv = &(padapter->cmdpriv); > - > + struct drvextra_cmd_parm *extra_parm = NULL; Don't do this. It disables GCC's uninitialized variable check so it can hide bugs. It's just another assignment to read and remember so it takes reviewer time. > > thread_enter("RTW_CMD_THREAD"); > > @@ -323,6 +323,11 @@ _next: > > if (_FAIL == rtw_cmd_filter(pcmdpriv, pcmd)) { > pcmd->res = H2C_DROPPED; > + if (pcmd->cmdcode == GEN_CMD_CODE(_Set_Drv_Extra)) { > + extra_parm = (struct drvextra_cmd_parm > *)pcmd->parmbuf; > + if (extra_parm && extra_parm->pbuf && > extra_parm->size > 0) > + rtw_mfree(extra_parm->pbuf, > extra_parm->size); Like Greg says, there isn't a rtw_mfree() anymore. This code is so confusing and GEN_CMD_CODE is horrible and "make drivers/staging/rtl8188eu/core/rtw_cmd.i" doesn't work and I don't know how to even review this... :/ But I'll try again when you re-submit. > + } > goto post_process; > } > regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [staging][r8188eu]: memory leak in rtw_free_cmd_obj if command is (_Set_Drv_Extra)
On Tue, Mar 18, 2014 at 10:25:27AM -0400, Wang, Xiaoming wrote: pcmd-parmbuf-pbuf has been allocated if command is GEN_CMD_CODE(_Set_Drv_Extra), and it enqueued by rtw_enqueue_cmd. rtw_cmd_thread dequeue pcmd by rtw_dequeue_cmd. The memory leak happened on this branch if( _FAIL == rtw_cmd_filter(pcmdpriv, pcmd) ) which goto post_process directly against freeing pcmd-parmbuf-pbuf in rtw_drvextra_cmd_hdl which is the cmd_hdl if command is (_Set_Drv_Extra). This patch free pcmd-parmbuf-pbuf on the forgotten branch to avoid memory leak. Signed-off-by: Zhang Dongxing dongxing.zh...@intel.com Signed-off-by: xiaoming wang xiaoming.w...@intel.com diff --git a/drivers/staging/rtl8188eu/core/rtw_cmd.c b/drivers/staging/rtl8188eu/core/rtw_cmd.c index c0a0a52..1c7f505 100644 --- a/drivers/staging/rtl8188eu/core/rtw_cmd.c +++ b/drivers/staging/rtl8188eu/core/rtw_cmd.c @@ -288,7 +288,7 @@ int rtw_cmd_thread(void *context) void (*pcmd_callback)(struct adapter *dev, struct cmd_obj *pcmd); struct adapter *padapter = (struct adapter *)context; struct cmd_priv *pcmdpriv = (padapter-cmdpriv); - + struct drvextra_cmd_parm *extra_parm = NULL; Don't do this. It disables GCC's uninitialized variable check so it can hide bugs. It's just another assignment to read and remember so it takes reviewer time. thread_enter(RTW_CMD_THREAD); @@ -323,6 +323,11 @@ _next: if (_FAIL == rtw_cmd_filter(pcmdpriv, pcmd)) { pcmd-res = H2C_DROPPED; + if (pcmd-cmdcode == GEN_CMD_CODE(_Set_Drv_Extra)) { + extra_parm = (struct drvextra_cmd_parm *)pcmd-parmbuf; + if (extra_parm extra_parm-pbuf extra_parm-size 0) + rtw_mfree(extra_parm-pbuf, extra_parm-size); Like Greg says, there isn't a rtw_mfree() anymore. This code is so confusing and GEN_CMD_CODE is horrible and make drivers/staging/rtl8188eu/core/rtw_cmd.i doesn't work and I don't know how to even review this... :/ But I'll try again when you re-submit. + } goto post_process; } regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: r8188eu: memory leak in rtw_free_cmd_obj if command is (_Set_Drv_Extra)
On Tue, Mar 18, 2014 at 10:25:27AM -0400, Wang, Xiaoming wrote: > pcmd->parmbuf->pbuf has been allocated if command is > GEN_CMD_CODE(_Set_Drv_Extra), > and it enqueued by rtw_enqueue_cmd. rtw_cmd_thread dequeue pcmd by > rtw_dequeue_cmd. > The memory leak happened on this branch "if( _FAIL == > rtw_cmd_filter(pcmdpriv, pcmd) )" > which goto post_process directly against freeing pcmd->parmbuf->pbuf in > rtw_drvextra_cmd_hdl which is the cmd_hdl if command is (_Set_Drv_Extra). > This patch free pcmd->parmbuf->pbuf on the forgotten branch to avoid memory > leak. > > Signed-off-by: Zhang Dongxing > Signed-off-by: xiaoming wang You obviously didn't test this as it breaks the build :( {sigh} Please retry after 3.15-rc1 is out. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: r8188eu: memory leak in rtw_free_cmd_obj if command is (_Set_Drv_Extra)
On Tue, Mar 18, 2014 at 10:25:27AM -0400, Wang, Xiaoming wrote: pcmd-parmbuf-pbuf has been allocated if command is GEN_CMD_CODE(_Set_Drv_Extra), and it enqueued by rtw_enqueue_cmd. rtw_cmd_thread dequeue pcmd by rtw_dequeue_cmd. The memory leak happened on this branch if( _FAIL == rtw_cmd_filter(pcmdpriv, pcmd) ) which goto post_process directly against freeing pcmd-parmbuf-pbuf in rtw_drvextra_cmd_hdl which is the cmd_hdl if command is (_Set_Drv_Extra). This patch free pcmd-parmbuf-pbuf on the forgotten branch to avoid memory leak. Signed-off-by: Zhang Dongxing dongxing.zh...@intel.com Signed-off-by: xiaoming wang xiaoming.w...@intel.com You obviously didn't test this as it breaks the build :( {sigh} Please retry after 3.15-rc1 is out. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [staging][r8188eu]: memory leak in rtw_free_cmd_obj if command is (_Set_Drv_Extra)
On Mon, Mar 10, 2014 at 11:37:44AM -0400, Wang, Xiaoming wrote: > pcmd->parmbuf->pbuf has been allocated if command is > GEN_CMD_CODE(_Set_Drv_Extra), > and it enqueued by rtw_enqueue_cmd. rtw_cmd_thread dequeue pcmd by > rtw_dequeue_cmd. > The memory leak happened on this branch "if( _FAIL == > rtw_cmd_filter(pcmdpriv, pcmd) )" > which goto post_process directly against freeing pcmd->parmbuf->pbuf in > rtw_drvextra_cmd_hdl which is the cmd_hdl if command is (_Set_Drv_Extra). > This patch free pcmd->parmbuf->pbuf on the forgotten branch to avoid memory > leak. > > Signed-off-by: Zhang Dongxing > Signed-off-by: xiaoming wang > Reviewed-by: Chuansheng Liu > > diff --git a/drivers/staging/rtl8188eu/core/rtw_cmd.c > b/drivers/staging/rtl8188eu/core/rtw_cmd.c > index c0a0a52..1c7f505 100644 > --- a/drivers/staging/rtl8188eu/core/rtw_cmd.c > +++ b/drivers/staging/rtl8188eu/core/rtw_cmd.c > @@ -288,7 +288,7 @@ int rtw_cmd_thread(void *context) > void (*pcmd_callback)(struct adapter *dev, struct cmd_obj *pcmd); > struct adapter *padapter = (struct adapter *)context; > struct cmd_priv *pcmdpriv = &(padapter->cmdpriv); > - > + struct drvextra_cmd_parm *extra_parm = NULL; > > thread_enter("RTW_CMD_THREAD"); > > @@ -323,6 +323,11 @@ _next: > > if (_FAIL == rtw_cmd_filter(pcmdpriv, pcmd)) { > pcmd->res = H2C_DROPPED; > + if (pcmd->cmdcode == GEN_CMD_CODE(_Set_Drv_Extra)) { > + extra_parm = (struct drvextra_cmd_parm > *)pcmd->parmbuf; > + if (extra_parm && extra_parm->pbuf && > extra_parm->size > 0) > + rtw_mfree(extra_parm->pbuf, > extra_parm->size); > + } > goto post_process; > } > This patch has all the tabs converted to spaces and can not be applied :( Please fix your email client and try again. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [staging][r8188eu]: memory leak in rtw_free_cmd_obj if command is (_Set_Drv_Extra)
On Mon, Mar 10, 2014 at 11:37:44AM -0400, Wang, Xiaoming wrote: pcmd-parmbuf-pbuf has been allocated if command is GEN_CMD_CODE(_Set_Drv_Extra), and it enqueued by rtw_enqueue_cmd. rtw_cmd_thread dequeue pcmd by rtw_dequeue_cmd. The memory leak happened on this branch if( _FAIL == rtw_cmd_filter(pcmdpriv, pcmd) ) which goto post_process directly against freeing pcmd-parmbuf-pbuf in rtw_drvextra_cmd_hdl which is the cmd_hdl if command is (_Set_Drv_Extra). This patch free pcmd-parmbuf-pbuf on the forgotten branch to avoid memory leak. Signed-off-by: Zhang Dongxing dongxing.zh...@intel.com Signed-off-by: xiaoming wang xiaoming.w...@intel.com Reviewed-by: Chuansheng Liu chuansheng@intel.com diff --git a/drivers/staging/rtl8188eu/core/rtw_cmd.c b/drivers/staging/rtl8188eu/core/rtw_cmd.c index c0a0a52..1c7f505 100644 --- a/drivers/staging/rtl8188eu/core/rtw_cmd.c +++ b/drivers/staging/rtl8188eu/core/rtw_cmd.c @@ -288,7 +288,7 @@ int rtw_cmd_thread(void *context) void (*pcmd_callback)(struct adapter *dev, struct cmd_obj *pcmd); struct adapter *padapter = (struct adapter *)context; struct cmd_priv *pcmdpriv = (padapter-cmdpriv); - + struct drvextra_cmd_parm *extra_parm = NULL; thread_enter(RTW_CMD_THREAD); @@ -323,6 +323,11 @@ _next: if (_FAIL == rtw_cmd_filter(pcmdpriv, pcmd)) { pcmd-res = H2C_DROPPED; + if (pcmd-cmdcode == GEN_CMD_CODE(_Set_Drv_Extra)) { + extra_parm = (struct drvextra_cmd_parm *)pcmd-parmbuf; + if (extra_parm extra_parm-pbuf extra_parm-size 0) + rtw_mfree(extra_parm-pbuf, extra_parm-size); + } goto post_process; } This patch has all the tabs converted to spaces and can not be applied :( Please fix your email client and try again. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] [staging][r8188eu]: memory leak in rtw_free_cmd_obj if command is (_Set_Drv_Extra)
Hi, > -Original Message- > From: Wang, Xiaoming > Sent: Monday, March 10, 2014 11:38 PM > To: gre...@linuxfoundation.org; valentina.mane...@gmail.com; > dan.carpen...@oracle.com; standby2...@gmail.com > Cc: de...@driverdev.osuosl.org; linux-kernel@vger.kernel.org; Zhang, > Dongxing; Wang, Xiaoming; Liu, Chuansheng > Subject: [PATCH] [staging][r8188eu]: memory leak in rtw_free_cmd_obj if > command is (_Set_Drv_Extra) > > pcmd->parmbuf->pbuf has been allocated if command is > GEN_CMD_CODE(_Set_Drv_Extra), > and it enqueued by rtw_enqueue_cmd. rtw_cmd_thread dequeue pcmd by > rtw_dequeue_cmd. > The memory leak happened on this branch "if( _FAIL == > rtw_cmd_filter(pcmdpriv, pcmd) )" > which goto post_process directly against freeing pcmd->parmbuf->pbuf in > rtw_drvextra_cmd_hdl which is the cmd_hdl if command is (_Set_Drv_Extra). > This patch free pcmd->parmbuf->pbuf on the forgotten branch to avoid memory > leak. > > Signed-off-by: Zhang Dongxing > Signed-off-by: xiaoming wang > > diff --git a/drivers/staging/rtl8188eu/core/rtw_cmd.c > b/drivers/staging/rtl8188eu/core/rtw_cmd.c > index c0a0a52..1c7f505 100644 > --- a/drivers/staging/rtl8188eu/core/rtw_cmd.c > +++ b/drivers/staging/rtl8188eu/core/rtw_cmd.c > @@ -288,7 +288,7 @@ int rtw_cmd_thread(void *context) > void (*pcmd_callback)(struct adapter *dev, struct cmd_obj *pcmd); > struct adapter *padapter = (struct adapter *)context; > struct cmd_priv *pcmdpriv = &(padapter->cmdpriv); > - > + struct drvextra_cmd_parm *extra_parm = NULL; > > thread_enter("RTW_CMD_THREAD"); > > @@ -323,6 +323,11 @@ _next: > > if (_FAIL == rtw_cmd_filter(pcmdpriv, pcmd)) { > pcmd->res = H2C_DROPPED; > + if (pcmd->cmdcode == > GEN_CMD_CODE(_Set_Drv_Extra)) { > + extra_parm = (struct > drvextra_cmd_parm *)pcmd->parmbuf; > + if (extra_parm && extra_parm->pbuf > && extra_parm->size > 0) > + rtw_mfree(extra_parm->pbuf, > extra_parm->size); > + } > goto post_process; > } > Reviewed-by: Chuansheng Liu Thanks. Best Regards Chuansheng
RE: [PATCH] [staging][r8188eu]: memory leak in rtw_free_cmd_obj if command is (_Set_Drv_Extra)
Hi, -Original Message- From: Wang, Xiaoming Sent: Monday, March 10, 2014 11:38 PM To: gre...@linuxfoundation.org; valentina.mane...@gmail.com; dan.carpen...@oracle.com; standby2...@gmail.com Cc: de...@driverdev.osuosl.org; linux-kernel@vger.kernel.org; Zhang, Dongxing; Wang, Xiaoming; Liu, Chuansheng Subject: [PATCH] [staging][r8188eu]: memory leak in rtw_free_cmd_obj if command is (_Set_Drv_Extra) pcmd-parmbuf-pbuf has been allocated if command is GEN_CMD_CODE(_Set_Drv_Extra), and it enqueued by rtw_enqueue_cmd. rtw_cmd_thread dequeue pcmd by rtw_dequeue_cmd. The memory leak happened on this branch if( _FAIL == rtw_cmd_filter(pcmdpriv, pcmd) ) which goto post_process directly against freeing pcmd-parmbuf-pbuf in rtw_drvextra_cmd_hdl which is the cmd_hdl if command is (_Set_Drv_Extra). This patch free pcmd-parmbuf-pbuf on the forgotten branch to avoid memory leak. Signed-off-by: Zhang Dongxing dongxing.zh...@intel.com Signed-off-by: xiaoming wang xiaoming.w...@intel.com diff --git a/drivers/staging/rtl8188eu/core/rtw_cmd.c b/drivers/staging/rtl8188eu/core/rtw_cmd.c index c0a0a52..1c7f505 100644 --- a/drivers/staging/rtl8188eu/core/rtw_cmd.c +++ b/drivers/staging/rtl8188eu/core/rtw_cmd.c @@ -288,7 +288,7 @@ int rtw_cmd_thread(void *context) void (*pcmd_callback)(struct adapter *dev, struct cmd_obj *pcmd); struct adapter *padapter = (struct adapter *)context; struct cmd_priv *pcmdpriv = (padapter-cmdpriv); - + struct drvextra_cmd_parm *extra_parm = NULL; thread_enter(RTW_CMD_THREAD); @@ -323,6 +323,11 @@ _next: if (_FAIL == rtw_cmd_filter(pcmdpriv, pcmd)) { pcmd-res = H2C_DROPPED; + if (pcmd-cmdcode == GEN_CMD_CODE(_Set_Drv_Extra)) { + extra_parm = (struct drvextra_cmd_parm *)pcmd-parmbuf; + if (extra_parm extra_parm-pbuf extra_parm-size 0) + rtw_mfree(extra_parm-pbuf, extra_parm-size); + } goto post_process; } Reviewed-by: Chuansheng Liu chuansheng@intel.com Thanks. Best Regards Chuansheng
Re: [PATCH] [staging][r8188eu]: memory leak in rtw_free_cmd_obj if command is (_Set_Drv_Extra)
On Fri, Mar 07, 2014 at 03:01:34PM -0500, Wang, Xiaoming wrote: > pcmd->parmbuf->pbuf has been allocated if command is > GEN_CMD_CODE(_Set_Drv_Extra), > and it enqueued by rtw_enqueue_cmd. rtw_cmd_thread dequeue pcmd by > rtw_dequeue_cmd. > The memory leak happened on this branch "if( _FAIL == > rtw_cmd_filter(pcmdpriv, pcmd) )" > which goto post_process directly against freeing pcmd->parmbuf->pbuf in > rtw_drvextra_cmd_hdl which is the cmd_hdl if command is (_Set_Drv_Extra). > This patch free pcmd->parmbuf->pbuf on forgotten branch to avoid memory leak. > > Signed-off-by: Zhang Dongxing > Signed-off-by: xiaoming wang This code looks completely different in linux-next. Please redo on top of linux-next if it's still needed. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [staging][r8188eu]: memory leak in rtw_free_cmd_obj if command is (_Set_Drv_Extra)
On Fri, Mar 07, 2014 at 03:01:34PM -0500, Wang, Xiaoming wrote: pcmd-parmbuf-pbuf has been allocated if command is GEN_CMD_CODE(_Set_Drv_Extra), and it enqueued by rtw_enqueue_cmd. rtw_cmd_thread dequeue pcmd by rtw_dequeue_cmd. The memory leak happened on this branch if( _FAIL == rtw_cmd_filter(pcmdpriv, pcmd) ) which goto post_process directly against freeing pcmd-parmbuf-pbuf in rtw_drvextra_cmd_hdl which is the cmd_hdl if command is (_Set_Drv_Extra). This patch free pcmd-parmbuf-pbuf on forgotten branch to avoid memory leak. Signed-off-by: Zhang Dongxing dongxing.zh...@intel.com Signed-off-by: xiaoming wang xiaoming.w...@intel.com This code looks completely different in linux-next. Please redo on top of linux-next if it's still needed. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/