[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240122210538.GJ126470@kernel.org>
Date: Mon, 22 Jan 2024 21:05:38 +0000
From: Simon Horman <horms@...nel.org>
To: Zhipeng Lu <alexious@....edu.cn>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Taku Izumi <izumi.taku@...fujitsu.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fjes: fix memleaks in fjes_hw_setup
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@....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@...nel.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
>
Powered by blists - more mailing lists