Received-SPF: pass (google.com: domain of linux-kernel+bounces-164413-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 30 Apr 2024 17:45:40 +0100
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Li Zhijian <lizhijian@fujitsu.com>
CC: <dave@stgolabs.net>, <dave.jiang@intel.com>, <alison.schofield@intel.com>,
	<vishal.l.verma@intel.com>, <ira.weiny@intel.com>,
	<dan.j.williams@intel.com>, <linux-cxl@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] cxl/region: Fix cxlr_pmem leaks
Message-ID: <20240430174540.000039ce@Huawei.com>
In-Reply-To: <20240428030748.318985-1-lizhijian@fujitsu.com>
References: <20240428030748.318985-1-lizhijian@fujitsu.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

On Sun, 28 Apr 2024 11:07:48 +0800
Li Zhijian <lizhijian@fujitsu.com> wrote:

> Before this error path, cxlr_pmem pointed to a kzalloc() memory, free
> it to avoid this memory leaking.
> 
> Fixes: f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Fix is good, but this looks like nice case for conversion to cleanup.h stuff
perhaps better to just do that?  Would need a small amount of dancing
on the final return to return cxlr->cxlr_pmem + pointer steal
when setting cxlr->cxlr_pmem a few lines up.

Also guard for the rwsem.

Dave, Dan, worth doing or take this minimal fix and spin around later?

If you think this is the way to go.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/region.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c186e0a39b9..812b2948b6c6 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2719,6 +2719,7 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
>  		if (i == 0) {
>  			cxl_nvb = cxl_find_nvdimm_bridge(cxlmd);
>  			if (!cxl_nvb) {
> +				kfree(cxlr_pmem);
>  				cxlr_pmem = ERR_PTR(-ENODEV);
>  				goto out;
>  			}