linux-kernel - Re: [RFC PATCH v1 2/2] cxl/cli: enforce HPA-descending teardown order for destroy-region

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aS-5Zjhz-5Mkdqpy@aschofie-mobl2.lan>
Date: Tue, 2 Dec 2025 20:15:34 -0800
From: Alison Schofield <alison.schofield@...el.com>
To: Pawel Mielimonka <pawel.mielimonka@...itsu.com>
CC: <dan.j.williams@...el.com>, <Smita.KoralahalliChannabasappa@....com>,
	<linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<dave@...olabs.net>, <jonathan.cameron@...wei.com>, <dave.jiang@...el.com>,
	<vishal.l.verma@...el.com>, <ira.weiny@...el.com>
Subject: Re: [RFC PATCH v1 2/2] cxl/cli: enforce HPA-descending teardown
 order for destroy-region

On Tue, Nov 25, 2025 at 11:38:24PM +0900, Pawel Mielimonka wrote:
> Implement destroy_multiple_regions() and bypass the generic
> do_region_xable() path for ACTION_DESTROY. Regions are collected and
> sorted by HPA, then destroyed from highest to lowest, stopping at the
> first "matching after skipped" to provide user with better error log.
> This prevents attempts on non-last regions and aligns destroy-region
> with required decoder programming order.

It would be useful to add a sample bad spew or what happens now
on attempt to destroy out of order.  Folks sometimes search on
those strings.


> 
> Signed-off-by: Pawel Mielimonka <pawel.mielimonka@...itsu.com>
> ---
>  cxl/region.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 58765b3d..1bf1901a 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -950,6 +950,57 @@ static int destroy_region(struct cxl_region *region)
>  	return cxl_region_delete(region);
>  }
>  
> +static int destroy_multiple_regions(struct parsed_params *p,
> +				 struct cxl_decoder *decoder,
> +				 int *count)
> +{
> +	struct cxl_region **list;
> +	int nr, rc, i;
> +	bool skipped = false;
> +
> +	rc = collect_regions_sorted(decoder, NULL, &list, &nr);
> +	if (rc) {
> +		log_err(&rl, "failed to allocate region list: %s\n", strerror(-rc));
> +		return rc;
> +	}
> +
> +	for (i = nr - 1; i >= 0; --i) {
> +		struct cxl_region *region = NULL;
> +

Here is where there is a difference btw 'all' and a decoder. 'All' gets
passed as an argument and the filter function recognizes it. But for the
by decoder option: "cxl destroy-region -f -d decoder0.2" argc=0 needs
special handling

Inserting this here worked for me:
+               /* If no region arguments provided, match all regions */
+               if (p->argc == 0)
+                       region = list[i];
+

Then with argc == 0 this next loop is a no-op but that is OK because
region is now assigned.


> +		for (int j = 0; j < p->argc; j++) {
> +			region = util_cxl_region_filter(list[i], p->argv[j]);
> +			if (region)
> +				break;
> +		}
> +
> +		if (!region) {
> +			skipped = true;
> +			continue;
> +		}
> +
> +		/* if current region matches filter, but previous didn't, destroying would
> +		 * result in breaking HPA continuity
> +		 */

Use kernel comment style. See other samples in this file.


> +		if (skipped) {
> +			log_err(&rl, "failed to destroy %s: not a valid HPA suffix under %s\n",

I'm not familiar w the usage of 'suffix' in this context.
How about replace "not a valid HPA suffix under"
with "out of order decoder reset"


> +				cxl_region_get_devname(region),
> +				cxl_decoder_get_devname(decoder));
> +			rc = -EINVAL;
> +			break;
> +		}
> +
> +		rc = destroy_region(region);
> +		if (rc) {
> +			log_err(&rl, "%s: failed: %s\n",
> +				cxl_region_get_devname(region), strerror(-rc));
> +			break;
> +		}
> +		++(*count);
> +	}
> +	free(list);
> +	return rc;
> +}
> +
>  static int do_region_xable(struct cxl_region *region, enum region_actions action)
>  {
>  	switch (action) {
> @@ -957,8 +1008,6 @@ static int do_region_xable(struct cxl_region *region, enum region_actions action
>  		return cxl_region_enable(region);
>  	case ACTION_DISABLE:
>  		return disable_region(region);
> -	case ACTION_DESTROY:
> -		return destroy_region(region);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -1026,7 +1075,12 @@ static int region_action(int argc, const char **argv, struct cxl_ctx *ctx,
>  			if (!util_cxl_decoder_filter(decoder,
>  						     param.root_decoder))
>  				continue;
> -			rc = decoder_region_action(p, decoder, action, count);
> +
> +			if (action == ACTION_DESTROY)
> +				rc = destroy_multiple_regions(p, decoder, count);
> +			else
> +				rc = decoder_region_action(p, decoder, action, count);
> +
>  			if (rc)
>  				err_rc = rc;
>  		}
> -- 
> 2.45.1.windows.1
>