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@xxxxxxxxxx>

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@xxxxxxxxxx>

..

> @@ -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
>