| Message ID | 20240122172445.3841883-1-alexious@zju.edu.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f6cc4b6a3ae53df425771000e9c9540cce9b7bb1 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fjes: fix memleaks in fjes_hw_setup | expand |
On Tue, Jan 23, 2024 at 01:24:42AM +0800, Zhipeng Lu wrote: > In fjes_hw_setup, it allocates several memory and delay the deallocation > to the fjes_hw_exit in fjes_probe through the following call chain: > > fjes_probe > |-> fjes_hw_init > |-> fjes_hw_setup > |-> fjes_hw_exit > > However, when fjes_hw_setup fails, fjes_hw_exit won't be called and thus > all the resources allocated in fjes_hw_setup will be leaked. In this > patch, we free those resources in fjes_hw_setup and prevents such leaks. > > Fixes: 2fcbca687702 ("fjes: platform_driver's .probe and .remove routine") > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> Hi Zhipeng Lu, It looks like the last non-trivial change to this driver was in 2016. So perhaps it is better to leave it be. But if not, this patch does look correct to me. Reviewed-by: Simon Horman <horms@kernel.org> ... > @@ -273,6 +277,25 @@ static int fjes_hw_setup(struct fjes_hw *hw) > fjes_hw_init_command_registers(hw, ¶m); > > return 0; > + > +free_epbuf: > + for (epidx = 0; epidx < hw->max_epid ; epidx++) { > + if (epidx == hw->my_epid) > + continue; > + fjes_hw_free_epbuf(&hw->ep_shm_info[epidx].tx); > + fjes_hw_free_epbuf(&hw->ep_shm_info[epidx].rx); > + } > + fjes_hw_free_shared_status_region(hw); > +free_res_buf: > + kfree(hw->hw_info.res_buf); > + hw->hw_info.res_buf = NULL; > +free_req_buf: > + kfree(hw->hw_info.req_buf); > + hw->hw_info.req_buf = NULL; > +free_ep_info: > + kfree(hw->ep_shm_info); > + hw->ep_shm_info = NULL; > + return result; FWIIW, I'm not sure it is necessary to set these pointers NULL, although it doesn't do any harm. Also, if this function returns an error, does the caller (fjes_hw_init()) leak hw->hw_info.trace? > } > > static void fjes_hw_cleanup(struct fjes_hw *hw) > -- > 2.34.1 >
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 23 Jan 2024 01:24:42 +0800 you wrote: > In fjes_hw_setup, it allocates several memory and delay the deallocation > to the fjes_hw_exit in fjes_probe through the following call chain: > > fjes_probe > |-> fjes_hw_init > |-> fjes_hw_setup > |-> fjes_hw_exit > > [...] Here is the summary with links: - fjes: fix memleaks in fjes_hw_setup https://git.kernel.org/netdev/net/c/f6cc4b6a3ae5 You are awesome, thank you!
> On Tue, Jan 23, 2024 at 01:24:42AM +0800, Zhipeng Lu wrote: > > In fjes_hw_setup, it allocates several memory and delay the deallocation > > to the fjes_hw_exit in fjes_probe through the following call chain: > > > > fjes_probe > > |-> fjes_hw_init > > |-> fjes_hw_setup > > |-> fjes_hw_exit > > > > However, when fjes_hw_setup fails, fjes_hw_exit won't be called and thus > > all the resources allocated in fjes_hw_setup will be leaked. In this > > patch, we free those resources in fjes_hw_setup and prevents such leaks. > > > > Fixes: 2fcbca687702 ("fjes: platform_driver's .probe and .remove routine") > > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> > > Hi Zhipeng Lu, > > It looks like the last non-trivial change to this driver was in 2016. > So perhaps it is better to leave it be. > > But if not, this patch does look correct to me. > > Reviewed-by: Simon Horman <horms@kernel.org> I think this patch doesn't change a lot since it just refactor the deallocation ways into unwind ladders while fix a memleak. > > ... > > > @@ -273,6 +277,25 @@ static int fjes_hw_setup(struct fjes_hw *hw) > > fjes_hw_init_command_registers(hw, ¶m); > > > > return 0; > > + > > +free_epbuf: > > + for (epidx = 0; epidx < hw->max_epid ; epidx++) { > > + if (epidx == hw->my_epid) > > + continue; > > + fjes_hw_free_epbuf(&hw->ep_shm_info[epidx].tx); > > + fjes_hw_free_epbuf(&hw->ep_shm_info[epidx].rx); > > + } > > + fjes_hw_free_shared_status_region(hw); > > +free_res_buf: > > + kfree(hw->hw_info.res_buf); > > + hw->hw_info.res_buf = NULL; > > +free_req_buf: > > + kfree(hw->hw_info.req_buf); > > + hw->hw_info.req_buf = NULL; > > +free_ep_info: > > + kfree(hw->ep_shm_info); > > + hw->ep_shm_info = NULL; > > + return result; > > FWIIW, I'm not sure it is necessary to set these pointers NULL, > although it doesn't do any harm. I set these pointers to NULL since its clean up function fjes_hw_cleanup do so. Personally, I tend to following the existing code style in the same module. > > Also, if this function returns an error, > does the caller (fjes_hw_init()) leak hw->hw_info.trace? Well, yes, it's a little bit wired that fjes_hw_init doesn't handle errors of fjes_hw_setup and vzalloc of hw->hw_info.trace as normal functions do. Maybe another patch need to be submitted to deal with this problem. > > > } > > > > static void fjes_hw_cleanup(struct fjes_hw *hw) > > -- > > 2.34.1 > >
diff --git a/drivers/net/fjes/fjes_hw.c b/drivers/net/fjes/fjes_hw.c index 704e949484d0..b9b5554ea862 100644 --- a/drivers/net/fjes/fjes_hw.c +++ b/drivers/net/fjes/fjes_hw.c @@ -221,21 +221,25 @@ static int fjes_hw_setup(struct fjes_hw *hw) mem_size = FJES_DEV_REQ_BUF_SIZE(hw->max_epid); hw->hw_info.req_buf = kzalloc(mem_size, GFP_KERNEL); - if (!(hw->hw_info.req_buf)) - return -ENOMEM; + if (!(hw->hw_info.req_buf)) { + result = -ENOMEM; + goto free_ep_info; + } hw->hw_info.req_buf_size = mem_size; mem_size = FJES_DEV_RES_BUF_SIZE(hw->max_epid); hw->hw_info.res_buf = kzalloc(mem_size, GFP_KERNEL); - if (!(hw->hw_info.res_buf)) - return -ENOMEM; + if (!(hw->hw_info.res_buf)) { + result = -ENOMEM; + goto free_req_buf; + } hw->hw_info.res_buf_size = mem_size; result = fjes_hw_alloc_shared_status_region(hw); if (result) - return result; + goto free_res_buf; hw->hw_info.buffer_share_bit = 0; hw->hw_info.buffer_unshare_reserve_bit = 0; @@ -246,11 +250,11 @@ static int fjes_hw_setup(struct fjes_hw *hw) result = fjes_hw_alloc_epbuf(&buf_pair->tx); if (result) - return result; + goto free_epbuf; result = fjes_hw_alloc_epbuf(&buf_pair->rx); if (result) - return result; + goto free_epbuf; spin_lock_irqsave(&hw->rx_status_lock, flags); fjes_hw_setup_epbuf(&buf_pair->tx, mac, @@ -273,6 +277,25 @@ static int fjes_hw_setup(struct fjes_hw *hw) fjes_hw_init_command_registers(hw, ¶m); return 0; + +free_epbuf: + for (epidx = 0; epidx < hw->max_epid ; epidx++) { + if (epidx == hw->my_epid) + continue; + fjes_hw_free_epbuf(&hw->ep_shm_info[epidx].tx); + fjes_hw_free_epbuf(&hw->ep_shm_info[epidx].rx); + } + fjes_hw_free_shared_status_region(hw); +free_res_buf: + kfree(hw->hw_info.res_buf); + hw->hw_info.res_buf = NULL; +free_req_buf: + kfree(hw->hw_info.req_buf); + hw->hw_info.req_buf = NULL; +free_ep_info: + kfree(hw->ep_shm_info); + hw->ep_shm_info = NULL; + return result; } static void fjes_hw_cleanup(struct fjes_hw *hw)
In fjes_hw_setup, it allocates several memory and delay the deallocation to the fjes_hw_exit in fjes_probe through the following call chain: fjes_probe |-> fjes_hw_init |-> fjes_hw_setup |-> fjes_hw_exit However, when fjes_hw_setup fails, fjes_hw_exit won't be called and thus all the resources allocated in fjes_hw_setup will be leaked. In this patch, we free those resources in fjes_hw_setup and prevents such leaks. Fixes: 2fcbca687702 ("fjes: platform_driver's .probe and .remove routine") Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> --- drivers/net/fjes/fjes_hw.c | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-)