Re: [PATCH v3 7/9] cxl/region: Fail region creation on position check mismatch

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Duplicate region target attach causes state corruption and NULL pointer dereference.
--

--- Patch [7]: [PATCH v3 7/9] cxl/region: Fail region creation on position check mismatch ---
commit 4352ee883f0aeb726a532c5b7f4c7ec6075bf005
Author: Alison Schofield <[email protected]>

cxl/region: Fail region creation on position check mismatch

This commit fails region creation when there is a mismatch between the
attach-order position and the auto-discovery calculator. By tearing down
targets before returning, it ensures configurations that cannot round-trip
through auto-discovery are not committed to CXL_CONFIG_ACTIVE.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index b4e8ebe158f7a..62179d987e4b5 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2492,6 +2492,29 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		rc = cxl_region_setup_targets(cxlr);
>  		if (rc)
>  			return rc;

[Severity: High]
This is a pre-existing issue, but does a duplicate target attach cause a NULL
pointer dereference here?

If a user writes the same endpoint decoder twice to a region target position,
the first write succeeds. On the second write, cxl_region_validate_position()
returns 0 because p->targets[pos] == cxled.

We then proceed to cxl_region_attach_position() which fails during xa_insert()
with -EEXIST. Its error rollback path unconditionally calls
cxl_port_detach_region() on all upstream ports, wiping out the cxl_rr port
tracking and freeing it if empty.

However, p->targets[pos] still holds the decoder.

Once the remaining targets are attached, this cxl_region_setup_targets() call
iterates over p->targets and calls cxl_port_setup_targets(), which attempts
to load the freed cxl_rr:

drivers/cxl/core/region.c:cxl_port_setup_targets() {
    ...
    struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
    ...
    struct cxl_decoder *cxld = cxl_rr->decoder;
    ...
}

Will this dereference the freed cxl_rr? Should cxl_region_attach() instead
short-circuit when cxl_region_validate_position() indicates the target is
already attached?

> +
> +		/*
> +		 * Verify that auto-discovery reconstructs the endpoint
> +		 * positions assigned while creating the region.
> +		 */

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7