Re: [PATCH] staging: rtl8723bs: fix memory leak error
On Tue, Aug 31, 2021 at 2:18 PM Dan Carpenter wrote: > > On Tue, Aug 31, 2021 at 01:03:55AM +0530, F.A.Sulaiman wrote: > > Smatch reported memory leak bug in rtl8723b_FirmwareDownload function. > > The problem is pFirmware memory is not released in release_fw1. > > Instead of redirecting to release_fw1 we can turn it into exit > > and free the memory. > > > > Signed-off-by: F.A. SULAIMAN > > --- > > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c > > b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c > > index de8caa6cd418..b59c2aa3a9d8 100644 > > --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c > > +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c > > @@ -436,7 +436,7 @@ s32 rtl8723b_FirmwareDownload(struct adapter *padapter, > > bool bUsedWoWLANFw) > > if (pFirmware->fw_length > FW_8723B_SIZE) { > > rtStatus = _FAIL; > > DBG_871X_LEVEL(_drv_emerg_, "Firmware size:%u exceed %u\n", > > pFirmware->fw_length, FW_8723B_SIZE); > > - goto release_fw1; > > + goto exit; > > } > > The current tree doesn't have DBG_871X_LEVEL() so you must be working > against something old. You need to work against linux-next or staging > next. > > Your patch fixes a bug, but it would be better to just re-write the > error handling for this function. There is another bug that a bunch > of error paths don't call release_firmware(fw). Use the "Free the Last > Thing" method. > > pFirmware = kzalloc(sizeof(struct rt_firmware), GFP_KERNEL); > if (!pFirmware) > return _FAIL; > > The last thing we allocated is "pFirmware" so free that if we have an > error. > > pBTFirmware = kzalloc(sizeof(struct rt_firmware), GFP_KERNEL); > if (!pBTFirmware) { > rtStatus = _FAIL; > goto free_firmware; > } > > Now the last thing is pBTFirmware. > > rtStatus = request_firmware(, fwfilepath, device); > if (rtStatus) { > rtStatus = _FAIL; > goto free_bt_firmware; > } > > Now the last thing is "fw". But this is a bit tricky because we're > going to release it as soon as possible and not wait until the end of > the function. There isn't a reason for this... We can change that or > keep it as-is. If we keep it as is, then the we'll just call > release_firmware(fw); before the goto free_bt_firmware; The current > code leaks fw on a bunch of error paths. > > pFirmware->fw_buffer_sz = kmemdup(fw->data, fw->size, GFP_KERNEL); > if (!pFirmware->fw_buffer_sz) { > rtStatus = _FAIL; > release_firmware(fw); > goto free_bt_firmware; > } > > Or: > > pFirmware->fw_buffer_sz = kmemdup(fw->data, fw->size, GFP_KERNEL); > if (!pFirmware->fw_buffer_sz) { > rtStatus = _FAIL; > goto release_fw; > } > > Now the last thing is pFirmware->fw_buffer_sz. Etc. > > Then at the end it's just: > > free_fw_buffer: > kfree(pFirmware->fw_buffer_sz); > release_fw: > release_firmware(fw); > free_bt_firmware: > kfree(pBTFirmware); > free_firmware: > kfree(pFirmware); > > return rtStatus; > } > Thank you! I'll do the changes and send you a v2 patch. Best Regards, Asha Sulaiman. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: fix memory leak error
On Tue, Aug 31, 2021 at 01:03:55AM +0530, F.A.Sulaiman wrote: > Smatch reported memory leak bug in rtl8723b_FirmwareDownload function. > The problem is pFirmware memory is not released in release_fw1. > Instead of redirecting to release_fw1 we can turn it into exit > and free the memory. > > Signed-off-by: F.A. SULAIMAN > --- > drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c > b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c > index de8caa6cd418..b59c2aa3a9d8 100644 > --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c > +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c > @@ -436,7 +436,7 @@ s32 rtl8723b_FirmwareDownload(struct adapter *padapter, > bool bUsedWoWLANFw) > if (pFirmware->fw_length > FW_8723B_SIZE) { > rtStatus = _FAIL; > DBG_871X_LEVEL(_drv_emerg_, "Firmware size:%u exceed %u\n", > pFirmware->fw_length, FW_8723B_SIZE); > - goto release_fw1; > + goto exit; > } The current tree doesn't have DBG_871X_LEVEL() so you must be working against something old. You need to work against linux-next or staging next. Your patch fixes a bug, but it would be better to just re-write the error handling for this function. There is another bug that a bunch of error paths don't call release_firmware(fw). Use the "Free the Last Thing" method. pFirmware = kzalloc(sizeof(struct rt_firmware), GFP_KERNEL); if (!pFirmware) return _FAIL; The last thing we allocated is "pFirmware" so free that if we have an error. pBTFirmware = kzalloc(sizeof(struct rt_firmware), GFP_KERNEL); if (!pBTFirmware) { rtStatus = _FAIL; goto free_firmware; } Now the last thing is pBTFirmware. rtStatus = request_firmware(, fwfilepath, device); if (rtStatus) { rtStatus = _FAIL; goto free_bt_firmware; } Now the last thing is "fw". But this is a bit tricky because we're going to release it as soon as possible and not wait until the end of the function. There isn't a reason for this... We can change that or keep it as-is. If we keep it as is, then the we'll just call release_firmware(fw); before the goto free_bt_firmware; The current code leaks fw on a bunch of error paths. pFirmware->fw_buffer_sz = kmemdup(fw->data, fw->size, GFP_KERNEL); if (!pFirmware->fw_buffer_sz) { rtStatus = _FAIL; release_firmware(fw); goto free_bt_firmware; } Or: pFirmware->fw_buffer_sz = kmemdup(fw->data, fw->size, GFP_KERNEL); if (!pFirmware->fw_buffer_sz) { rtStatus = _FAIL; goto release_fw; } Now the last thing is pFirmware->fw_buffer_sz. Etc. Then at the end it's just: free_fw_buffer: kfree(pFirmware->fw_buffer_sz); release_fw: release_firmware(fw); free_bt_firmware: kfree(pBTFirmware); free_firmware: kfree(pFirmware); return rtStatus; } regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl8723bs: fix memory leak error
Smatch reported memory leak bug in rtl8723b_FirmwareDownload function. The problem is pFirmware memory is not released in release_fw1. Instead of redirecting to release_fw1 we can turn it into exit and free the memory. Signed-off-by: F.A. SULAIMAN --- drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c index de8caa6cd418..b59c2aa3a9d8 100644 --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c @@ -436,7 +436,7 @@ s32 rtl8723b_FirmwareDownload(struct adapter *padapter, bool bUsedWoWLANFw) if (pFirmware->fw_length > FW_8723B_SIZE) { rtStatus = _FAIL; DBG_871X_LEVEL(_drv_emerg_, "Firmware size:%u exceed %u\n", pFirmware->fw_length, FW_8723B_SIZE); - goto release_fw1; + goto exit; } pFirmwareBuf = pFirmware->fw_buffer_sz; @@ -512,7 +512,6 @@ s32 rtl8723b_FirmwareDownload(struct adapter *padapter, bool bUsedWoWLANFw) exit: kfree(pFirmware->fw_buffer_sz); kfree(pFirmware); -release_fw1: kfree(pBTFirmware); DBG_871X(" <=== rtl8723b_FirmwareDownload()\n"); return rtStatus; -- 2.17.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel