Re: [PATCH v7 3/5] cxl/region: Support auto region granularity finer than the root

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] cxl/region: passthrough parent granularity used in
  cxl_port_calc_interleave()
- [Medium] cxl/region: unvalidated endpoint positions in
  cxl_region_sort_targets()
--

commit d69ce1cc3e00f111d2894f426382e5b9afaa5815
Author: Alison Schofield <alison.schofield@intel.com>

cxl/region: Support auto region granularity finer than the root

This commit adds support for assembling auto-discovered CXL regions whose
interleave granularity is finer than the interleaving root decoder, and
updates unaligned region offset-to-DPA translation to use the region
granularity and ways.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 1594bec3930bf..0a39e634b4b0a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c

[ ... ]

> @@ -1449,6 +1495,14 @@ static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr
>  	struct cxl_port *iter = port;
>  	int distance = 1;
>  
> +	/*
> +	 * A finer-than-root region refines toward the endpoints, so peer
> +	 * targets at the finest interleaving level occupy consecutive region
> +	 * positions.
> +	 */
> +	if (cxl_region_is_finer_than_root(cxlr))
> +		return 1;
> +
>  	do {
>  		cxl_rr_iter = cxl_rr_load(iter, cxlr);
>  		distance *= cxl_rr_iter->nr_targets;

[Severity: Medium]
Can check_last_peer() index out of bounds or dereference NULL in
p->targets[] when cxl_port_peer_distance() returns 1?

During auto-region assembly in cxl_region_attach(),
cxl_region_sort_targets() runs before cxl_region_setup_targets() validates
intermediate decoder granularities.  cxl_port_interleave_pos() scales pos
by gran / region_gran using the unvalidated decoder granularity, and
cxl_region_sort_targets() only checks cxled->pos < 0 without verifying
that the sorted positions match 0 .. p->nr_targets - 1:

drivers/cxl/core/region.c:cxl_region_sort_targets() {
	for (i = 0; i < p->nr_targets; i++) {
		struct cxl_endpoint_decoder *cxled = p->targets[i];

		cxled->pos = cxl_calc_interleave_pos(cxled, cxlr);
		...
		if (cxled->pos < 0)
			rc = -ENXIO;
	}
	/* Keep the cxlr target list in interleave position order */
	sort(p->targets, p->nr_targets, sizeof(p->targets[0]),
	     cmp_interleave_pos, NULL);
}

If firmware programs a bogus granularity on an intermediate decoder,
cxled->pos can exceed p->nr_targets or CXL_DECODER_MAX_INTERLEAVE (or
produce duplicate positions).  When cxl_region_setup_targets() then calls
check_last_peer() with distance == 1 at an upper port, pos < distance only
rejects pos < 1 before indexing p->targets[pos - 1] and dereferencing
cxled_peer.  Because cxl_region_attach_position() attaches endpoints using
the loop index i while cxled->pos != i, later teardown in
__cxl_decoder_detach() also hits dev_WARN_ONCE() and fails to detach the
endpoint.

Validating that p->targets[i]->pos == i for all targets after sort() in
cxl_region_sort_targets() would catch both out-of-range and duplicate
positions before attachment and target setup proceed.

> @@ -1476,15 +1530,17 @@ static int cxl_port_parent_interleave(struct cxl_port *port,
>  	int parent_iw, parent_ig;
>  
>  	if (is_cxl_root(parent_port)) {
> -		/*
> -		 * Root decoder IG is always set to value in CFMWS which
> -		 * may be different than this region's IG.  We can use the
> -		 * region's IG here since interleave_granularity_store()
> -		 * does not allow interleaved host-bridges with
> -		 * root IG != region IG.
> -		 */
> -		parent_ig = p->interleave_granularity;
>  		parent_iw = cxlrd->cxlsd.cxld.interleave_ways;
> +		if (parent_iw > 1) {
> +			parent_ig = cxlrd->cxlsd.cxld.interleave_granularity;
> +		} else {
> +			/*
> +			 * A single-way root does not interleave, so its
> +			 * granularity does not constrain the region interleave.
> +			 * Use the region granularity instead.
> +			 */
> +			parent_ig = p->interleave_granularity;
> +		}
>  		if (!is_power_of_2(parent_iw))
>  			parent_iw /= 3;
>  	} else {
> @@ -1521,7 +1577,7 @@ static int cxl_port_calc_interleave(struct cxl_port *port, struct cxl_region *cx
>  				    struct cxl_region_ref *cxl_rr, int *out_iw,
>  				    int *out_ig)
>  {
> -	struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
> +	struct cxl_region_params *p = &cxlr->params;
>  	struct cxl_decoder *cxld = cxl_rr->decoder;
>  	int parent_iw, parent_ig, iw, ig, rc;
>  	struct cxl_switch_decoder *cxlsd;
> @@ -1537,16 +1593,24 @@ static int cxl_port_calc_interleave(struct cxl_port *port, struct cxl_region *cx
>  		return -EINVAL;
>  	}
>  
> -	/*
> -	 * Interleave granularity is a multiple of @parent_port granularity.
> -	 * Multiplier is the parent port interleave ways.
> -	 */
> -	ig = parent_ig * parent_iw;
> +	/* Finer-than-root regions refine granularity toward the endpoints */
> +	if (cxl_region_is_finer_than_root(cxlr)) {
> +		ig = parent_ig / iw;
> +
> +		if (ig < p->interleave_granularity) {
> +			dev_dbg(&cxlr->dev,
> +				"%s:%s: granularity %d is finer than region granularity %d\n",
> +				dev_name(port->uport_dev), dev_name(&port->dev),
> +				ig, p->interleave_granularity);
> +			return -ENXIO;
> +		}
> +	} else {
> +		ig = parent_ig * parent_iw;
> +	}
>  
>  	if (!granularity_valid(ig)) {
> -		dev_dbg(&cxlr->dev,
> -			"%s: invalid granularity calculation (%d * %d)\n",
> -			dev_name(&parent_port->dev), parent_ig, parent_iw);
> +		dev_dbg(&cxlr->dev, "%s:%s: invalid granularity: %d\n",
> +			dev_name(port->uport_dev), dev_name(&port->dev), ig);
>  		return -EINVAL;
>  	}

[Severity: High]
Does cxl_port_calc_interleave() use an unvalidated passthrough decoder
granularity when an interleaving switch sits below a 1-way host bridge or
parent switch?

In cxl_port_setup_decoder(), an auto-region passthrough decoder (iw == 1)
is exempted from matching ig and its cxld->interleave_granularity is left
at whatever raw value firmware programmed in the hardware register (such
as 256B):

drivers/cxl/core/region.c:cxl_port_setup_decoder() {
	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
		if (cxld->interleave_ways != iw ||
		    (iw > 1 && cxld->interleave_granularity != ig) ||
		    !spa_maps_hpa(p, &cxld->hpa_range) ||
		    ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)) {
			...
			return -ENXIO;
		}
	}
}

When cxl_region_setup_targets() then descends to a downstream interleaving
switch port (for example, a 3-way root at 2 * IGB -> 1-way host bridge ->
2-way switch at IGB), the non-root branch of cxl_port_parent_interleave()
reads parent_ig = parent_cxld->interleave_granularity directly from that
passthrough host bridge decoder.  cxl_port_calc_interleave() then divides
that raw passthrough granularity by iw instead of using the granularity
from the nearest upstream interleaving decoder, causing auto-region
assembly to fail with -ENXIO.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794000806.git.alison.schofield@intel.com?part=3