[PATCH] fjes: fix memleaks in fjes_hw_setup

[PATCH] fjes: fix memleaks in fjes_hw_setup

[Zhipeng Lu]

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

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, &param);
 
 	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)
-- 
2.34.1

Re: [PATCH] fjes: fix memleaks in fjes_hw_setup

[Simon Horman]

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, &param);
>  
>  	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
> 

Re: [PATCH] fjes: fix memleaks in fjes_hw_setup

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: Re: [PATCH] fjes: fix memleaks in fjes_hw_setup

[alexious]

> 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, &param);
> >  
> >  	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
> >