Received-SPF: pass (google.com: domain of linux-kernel+bounces-34164-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 22 Jan 2024 21:05:38 +0000
From: Simon Horman <horms@kernel.org>
To: Zhipeng Lu <alexious@zju.edu.cn>
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Taku Izumi <izumi.taku@jp.fujitsu.com>, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fjes: fix memleaks in fjes_hw_setup
Message-ID: <20240122210538.GJ126470@kernel.org>
References: <20240122172445.3841883-1-alexious@zju.edu.cn>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240122172445.3841883-1-alexious@zju.edu.cn>

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
>