Received-SPF: pass (google.com: domain of linux-kernel+bounces-167873-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 3 May 2024 09:07:13 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: "Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com>, Ira Weiny
	<ira.weiny@intel.com>, "dave@stgolabs.net" <dave@stgolabs.net>,
	"jonathan.cameron@huawei.com" <Jonathan.Cameron@huawei.com>,
	"dave.jiang@intel.com" <dave.jiang@intel.com>, "alison.schofield@intel.com"
	<alison.schofield@intel.com>, "vishal.l.verma@intel.com"
	<vishal.l.verma@intel.com>, "dan.j.williams@intel.com"
	<dan.j.williams@intel.com>, "linux-cxl@vger.kernel.org"
	<linux-cxl@vger.kernel.org>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] cxl/region: Fix memregion leaks in devm_cxl_add_region()
Message-ID: <66350bb1d3817_1aefb294ba@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240428053715.327444-1-lizhijian@fujitsu.com>
 <662fc9659f6ab_19ca3929435@iweiny-mobl.notmuch>
 <TY1PR01MB1562E0A5EA4F95FFE0F39602A51F2@TY1PR01MB1562.jpnprd01.prod.outlook.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <TY1PR01MB1562E0A5EA4F95FFE0F39602A51F2@TY1PR01MB1562.jpnprd01.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?UrtCAGOCYG8aPnYAhHRdyU65+sz3Ci4+fPrJig1CYq5gX7dndUwM1IG/uMbX?=
 =?us-ascii?Q?Q0ixLyDWjXCgqKtApuiC9xad+m3ZlUfDhIFi8sabAxHIWuEP7aT8NBEZdb3z?=
 =?us-ascii?Q?i/ASLHSA6KMeh6mEHjvmI+aT9ajqKXuVfbpmCxDUnbYJ7k5ucXoUz7Qa4SkE?=
 =?us-ascii?Q?PHABb7jceeESRk1ukd0wfES/LzSvesImM0g1w8Ol2yHDbQmuAMYb2JcS5KYR?=
 =?us-ascii?Q?MoP44Z01jAkcMBNg4t+P+Q565k1DY1bvfX4Ob5EPqkCpQhHjbvOPWWIo78eh?=
 =?us-ascii?Q?vNlfP9i28rWeomDgrn03P/IKWNiauW72EBxpXDDGMf8poQyheukyePnoHNWS?=
 =?us-ascii?Q?UrhLL67Ssi/PBfI9CeMonGl4/OsksNP4MQt7Ow9aBZPL3kR6AxYnHxMg7Ia9?=
 =?us-ascii?Q?kiZ0XmjY2CWYzm0bWjPgOPGbZu6toc5/3zUWZcnZc1sdCbE+UsJhgrcp3Gra?=
 =?us-ascii?Q?a9xQN3WSwvLgCmy7N3nYPiXPnZcVk1c3Rs5WcLtzGB2UUpmrRWN4RChlNI14?=
 =?us-ascii?Q?u3czWaeJJT9z4a0NRS289DYcGdGPvjcBJmDZI1+9Za0wIZmTxStI0WpaIZDO?=
 =?us-ascii?Q?y+reXg/EwmwKZlSNhqI1G3rJjId47eGEv03DCqjkjrbpTnUko0LCZ02W5bLM?=
 =?us-ascii?Q?WkxOYSAyTC4JCRxXmLgU7wnZl+ZGE50dxDXjgeJnQsPMoiEN4xlJReSS0aJL?=
 =?us-ascii?Q?eQDlEaw6lTrw1W3f6LD0zycgJd+WfJRpks3SJZlyRx+O3GFXEKyZTc9irNgN?=
 =?us-ascii?Q?cJhLV0TqxRQIn2en9DBDEx+DDR42MtoAAm+5rvos0Sk/brETjZCXgETN7Ly9?=
 =?us-ascii?Q?VnUJ3nWtpVM/nS/UrklWse6FFsssdf4argc/RIXOn4F72g8qVZPgLwGQWlZJ?=
 =?us-ascii?Q?Vz9kZr7PI+Sbp882mtordH9Gtl8fSkAe6aYIIJ2B9kL2OyqQKwojPdfVGx/5?=
 =?us-ascii?Q?4HS59MSItuX4JRC8O3oBZ3J/xeFWJSliwcnMs3EXOqFjndqikcvxSqmjYmSz?=
 =?us-ascii?Q?v5xGaEL/zrUnwkFtpNolvaPNrSDxTy/Zef0mQrUz1IdT4ARaSGIcCCkNWrmF?=
 =?us-ascii?Q?p+KludYPWd1zGfWVMJOZm5I49IvJY+rzFHm8hfx4/MdqKlEdu2oqpWt0Dex5?=
 =?us-ascii?Q?w7YkyBI6kzjbBns/SkL8++jlV6t8LGt2ZE0i2G3Zmyq6vjC8PwJp7fzRry7u?=
 =?us-ascii?Q?qXD+PmE8igW5Y47ys3J7VOPybeQ1Ys5/hogpT02U/oZ2eMkdcNreSC3xWP7G?=
 =?us-ascii?Q?a+zE3Owl3KLEAvo+go3x3N3pam/67sGBOf4/zKA8L/4iUmyMNl15l0mJHcFm?=
 =?us-ascii?Q?iKPmadNZrj4+3ZBqaGqAikxsbax9pgTLMOMTpPlFTAnTDzgn86PiSABORPuj?=
 =?us-ascii?Q?x08PtBpIHyY1VWLcLOxECubX0CE42tMD6BadXb1OJMmM/dWtTveF652uOV2a?=
 =?us-ascii?Q?66lXsuTSwEz6ulNPv0ICvIccQWNM85X4ux081AK9aL/qBIV2G7oieBrvGond?=
 =?us-ascii?Q?YPtb5HaifMlDmCjYVrc5SD3Ll72cR4jt68FUJJLxUz5F4kOm1BC2ChUQ8SKT?=
 =?us-ascii?Q?DbE5hK0FWRUCTioy1rl6npJN6p9ODWgauws2DJRBmKgq94aCwG3Ny8DwVJgB?=
 =?us-ascii?Q?kw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0550dacc-14ef-41ef-4d1f-08dc6b8b1a17
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2024 16:07:16.3152
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: MuKpdOjtCBriN5YabzlSCwk86Bz/UtGQQf5PvTJqtzTfaOiQvSVunTErbkEZSSopIwDiUQqT1Qw3qNyRTiWf8V6fnP8VMsFwc2fmrBCocqI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4529
X-OriginatorOrg: intel.com

Zhijian Li (Fujitsu) wrote:
> > I'm not following what the problem is you are trying to fix.  This seems
> > like it just moves the memregion_free() around a bit but the logic is the
> > same.
> 
> This patch not only memregion_free(id) out of cxl_region_alloc() but
> also add an  extra memregion_free(id) in EINVAL case which led to a
> memory leak previously
> 

I think I would prefer to just eliminate that failure case, something
like:

-- >8 --
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 5c186e0a39b9..95ba32f8649c 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2352,15 +2352,6 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
        struct device *dev;
        int rc;
 
-       switch (mode) {
-       case CXL_DECODER_RAM:
-       case CXL_DECODER_PMEM:
-               break;
-       default:
-               dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode);
-               return ERR_PTR(-EINVAL);
-       }
-
        cxlr = cxl_region_alloc(cxlrd, id);
        if (IS_ERR(cxlr))
                return cxlr;
@@ -2415,6 +2406,15 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
 {
        int rc;
 
+       switch (mode) {
+       case CXL_DECODER_RAM:
+       case CXL_DECODER_PMEM:
+               break;
+       default:
+               dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode);
+               return ERR_PTR(-EINVAL);
+       }
+
        rc = memregion_alloc(GFP_KERNEL);
        if (rc < 0)
                return ERR_PTR(rc);