Re: [PATCH v1 12/14] cxl/region: Read existing extents on region creation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] cxl/mbox: per-extent DPA filtering in cxl_read_extent_list() splits tag groups
- [High] cxl/mbox: cxl_release_recovered() releases already-onlined DPA capacity
- [High] cxl/region: race in cxl_region_add_existing_extents() on concurrent detach
- [Medium] cxl/mbox: missing end < start check before range_contains() in extent scan
--

commit 6931a3e5d551f78995bded312bdac1337c547e3c
Author: Ira Weiny <iweiny@kernel.org>

cxl/region: Read existing extents on region creation

This patch reads pre-existing accepted dynamic capacity extents from the
device via CXL_MBOX_OP_GET_DC_EXTENT_LIST when probing a cxl_dax_region. It
filters extents by the endpoint decoder's DPA range, stages and onlines
recovered tag groups without sending an Add response, and gates new extent
add events until the initial scan completes.

> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index c3236fe9e7c5e..a18b41ff5d232 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
[ ... ]
> @@ -1627,6 +1641,10 @@ static int cxl_realize_group(struct cxl_memdev_state *mds, const uuid_t *tag,
>  	mds->dc_chain.group = NULL;
>  	if (tag_group)
>  		discard_tag_group(tag_group);
> +err_release:
> +	/* A fresh group is simply left out of the Add response */
> +	if (existing)
> +		cxl_release_recovered(mds, tag, group);
>  	return rc;
>  }
>  
> @@ -1686,11 +1704,14 @@ static int cxl_validate_group(struct cxl_memdev_state *mds, const uuid_t *tag,
>   * list.
>   */
>  static int cxl_add_pending(struct cxl_memdev_state *mds,
> -			   struct list_head *pending)
> +			   struct list_head *pending, bool existing)
>  {
[ ... ]
> @@ -1727,11 +1748,13 @@ static int cxl_add_pending(struct cxl_memdev_state *mds,
>  		shareable = cxl_group_is_shareable(mds, &group);
>  
>  		if (cxl_validate_group(mds, &tag, &group, shareable)) {
> +			if (existing)
> +				cxl_release_recovered(mds, &tag, &group);
>  			drop_extent_group(&group);
>  			continue;
>  		}
>  
> -		cnt = cxl_realize_group(mds, &tag, &group, shareable);
> +		cnt = cxl_realize_group(mds, &tag, &group, shareable, existing);

[Severity: High]
Could calling cxl_release_recovered() on all validation and realization
failures release DPA capacity that is already online on the host?

During recovery (existing == true), cxl_add_pending() processes groups
sequentially and onlines valid groups into cxlr_dax->dc_extents. If the
device's extent list contains a subsequent group whose tag is already
committed (cxl_tag_already_committed() returning true in
cxl_validate_group()) or whose extent DPA range duplicates or overlaps an
already-onlined dc_extent:

drivers/cxl/core/extent.c:cxl_add_extent() {
	...
	switch (cxlr_dax_classify_extent(cxlr_dax, cxled, &ext_range)) {
	case CXL_EXT_DUPLICATE:
		...
		return -EEXIST;
	case CXL_EXT_OVERLAP:
		return -ENXIO;
	case CXL_EXT_NEW:
		break;
	}
}

both error paths invoke cxl_release_recovered(), which sends
CXL_MBOX_OP_RELEASE_DC for every extent in the failing group. Wouldn't that
release physical capacity on the device that backs an active, already-onlined
dc_extent on the host?

> @@ -2439,6 +2471,180 @@ int cxl_dev_dc_identify(struct cxl_mailbox *mbox,
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_dev_dc_identify, "CXL");
>  
> +/*
> + * Stage the device's extents inside @dpa_range on @existing.  Returns
> + * -EAGAIN if the list changes while it is being read.
> + */
> +static int cxl_read_extent_list(struct cxl_memdev_state *mds,
> +				const struct range *dpa_range,
> +				struct list_head *existing)
> +{
[ ... ]
> +		for (int i = 0; i < nr_returned ; i++) {
> +			struct cxl_extent *extent = &extents->extent[i];
> +			struct range ext_range;
> +
> +			ext_range.start = le64_to_cpu(extent->start_dpa);
> +			ext_range.end = ext_range.start +
> +					le64_to_cpu(extent->length) - 1;
> +			dev_dbg(dev, "Processing extent %d/%d %pra (%pUb) seq %u\n",
> +				current_index + i, total_expected, &ext_range,
> +				extent->uuid,
> +				le16_to_cpu(extent->shared_extn_seq));
> +			/* Another region's, or an unmapped partition's: not ours */
> +			if (!range_contains(dpa_range, &ext_range))
> +				continue;
> +			rc = stage_pending_extent(existing, extent);

[Severity: Medium]
What happens in cxl_read_extent_list() if the device returns a malformed
extent where extent->length is 0 or start_dpa + length - 1 wraps around u64
such that ext_range.end < ext_range.start?

Because range_contains(r1, r2) only checks r1->start <= r2->start &&
r1->end >= r2->end, cxl_extent_dc_partition() explicitly guards against
ext_range->end < ext_range->start before calling range_contains(). Without a
similar check here, an extent with ext_range.start >= dpa_range->start (even
in a higher DPA partition) and a wrapped ext_range.end <= dpa_range->end
passes range_contains(dpa_range, &ext_range) and is staged onto existing.

If that malformed extent carries a UUID matching a valid tag group on this
region, extract_tag_group() will merge it into that group in
cxl_add_pending(), causing cxl_validate_group() to fail and
cxl_release_recovered() to release the entire valid group via
CXL_MBOX_OP_RELEASE_DC. Should cxl_read_extent_list() check
ext_range.end < ext_range.start before calling range_contains()?

[Severity: High]
Also in cxl_read_extent_list(), can filtering individual extents by
dpa_range before assembling tag groups split a multi-extent tag group that
spans or straddles the endpoint decoder's DPA boundary?

When cxl_read_extent_list() skips individual extents with
!range_contains(dpa_range, &ext_range), only the in-range subset of extents
is staged onto existing. When __cxl_process_extent_list() subsequently calls
cxl_add_pending(mds, &existing, true), extract_tag_group() forms each group
from only that pre-filtered subset:

1. For a non-shareable group (or a shareable group whose in-range extents
   happen to carry shared_extn_seq 0..k-1), cxl_validate_group() and
   cxl_realize_group() will succeed and online a truncated tag group,
   committing the tag UUID host-wide so the remaining extents cannot be added
   elsewhere.
2. For a shareable group whose in-range subset is missing earlier sequence
   numbers, cxl_check_group_seq() fails and cxl_release_recovered() sends
   CXL_MBOX_OP_RELEASE_DC for only the in-range subset, violating CXL r4.0
   Section 8.2.10.9.9.4 which requires shared tag groups to be released as a
   whole.

Should all extents be staged first (or grouped by tag first) so that a tag
group only partially contained in dpa_range is detected as a whole?

> diff --git a/drivers/cxl/core/region_dax.c b/drivers/cxl/core/region_dax.c
> index 790176a8aaddc..816386301b80e 100644
> --- a/drivers/cxl/core/region_dax.c
> +++ b/drivers/cxl/core/region_dax.c
[ ... ]
> @@ -79,6 +81,15 @@ static struct cxl_dax_region *cxl_dax_region_alloc(struct cxl_region *cxlr)
>  	cxlr_dax->hpa_range.start = p->res->start;
>  	cxlr_dax->hpa_range.end = p->res->end;
>  
> +	/*
> +	 * DC regions are not interleaved; the probe-time scan uses this decoder
> +	 * and may outlive its attachment to the region, so hold a reference.
> +	 */
> +	if (cxlr->mode == CXL_PARTMODE_DYNAMIC_RAM)
> +		cxlr_dax->cxled = p->targets[0];
> +	if (cxlr_dax->cxled)
> +		get_device(&cxlr_dax->cxled->cxld.dev);
> +
>  	dev = &cxlr_dax->dev;
>  	cxlr_dax->cxlr = cxlr;
>  	cxlr->cxlr_dax = cxlr_dax;
> @@ -153,6 +164,42 @@ DEFINE_FREE(unpublish_cxl_dax_region, struct cxl_dax_region *,
>  		put_device(&_T->dev);
>  	})
>  
> +/*
> + * Runs from cxl_dax_region_probe().  The region's parameters may not be
> + * read here: a concurrent detach clears p->targets[] under cxl_rwsem.region
> + * before it waits for this probe.  Use the decoder recorded at allocation
> + * instead; cxlr_dax holds a reference on it.
> + */
> +int cxl_region_add_existing_extents(struct cxl_dax_region *cxlr_dax)
> +{
> +	struct cxl_endpoint_decoder *cxled = cxlr_dax->cxled;
> +	int rc;
> +
> +	rc = cxl_process_extent_list(cxled);

[Severity: High]
Can a concurrent region detach or reset while cxl_dax_region_probe() runs
cause cxl_region_add_existing_extents() to race with decoder reuse, memdev
teardown, or cache invalidation?

In cxl_decoder_detach(), __cxl_decoder_detach() runs under cxl_rwsem.region
write lock, disables the decoder, and clears cxled->cxld.region = NULL and
p->targets[0] = NULL before dropping cxl_rwsem.region and calling
device_release_driver(&detach->dev) -> cxlr_dax_unregister(), which waits on
device_lock(&cxlr_dax->dev) for cxl_dax_region_probe() to finish. Because
cxl_region_add_existing_extents() passes only cxled to
cxl_process_extent_list() and runs without cxl_rwsem.region held across the
scan, it looks like several races can occur once __cxl_decoder_detach()
drops cxl_rwsem.region:

1. If cxled is reattached to a new region (cxlr2 / cxlr_dax2) while the old
   cxlr_dax1 probe is in cxl_read_extent_list(), cxl_add_pending() ->
   cxl_realize_group() -> cxl_add_extent() -> cxl_validate_extent() resolves
   start_dpa via cxl_dpa_to_region(cxlmd, start_dpa, &cxled) to cxlr_dax2
   without checking that cxlr->cxlr_dax == cxlr_dax1. Because existing is
   true, cxl_validate_extent() bypasses the extents_scanned gate and
   online_tag_group() attaches devres actions to cxlr_dax2->dev before
   cxlr_dax2 has probed, causing really_probe() on cxlr_dax2 to fail with
   -EBUSY ("Resources present before probing") and leak the tag groups.
2. Similarly, cxled->dpa_res can be freed and reallocated to a different DPA
   range via cxl_dpa_free() / cxl_dpa_alloc() before
   __cxl_process_extent_list() (or one of its -EAGAIN retries) reads
   cxled->dpa_res under cxl_rwsem.dpa, causing it to scan and release
   extents in the new DPA range via cxl_release_recovered().
3. Once detach_target() clears cxled->cxld.region = NULL, a concurrent
   memdev/PCI unbind running cxld_unregister() -> cxl_decoder_detach() sees
   cxled->cxld.region == NULL and returns NULL without waiting for
   cxlr_dax1's probe. Holding get_device(&cxled->cxld.dev) only pins the
   decoder device, not cxlmd->cxlds, which cxl_memdev_shutdown() sets to
   NULL before cxl_pci devres frees mds while cxl_process_extent_list() is
   dereferencing cxled_to_mds(cxled).
4. During commit_store() reset (or cxl_decoder_detach()),
   cxlr_dax_unpublish() clears cxlr->cxlr_dax = NULL before waiting for
   cxl_dax_region_probe() (and in commit_store(), before
   cxl_region_decode_reset() invalidates CPU caches). When the in-flight
   scan sees !cxlr->cxlr_dax, cxl_validate_extent() returns -ENXIO and
   cxl_realize_group() calls cxl_release_recovered(), sending
   CXL_MBOX_OP_RELEASE_DC while CPU caches may still hold dirty lines or
   before the region teardown has finished.

Should cxl_region_add_existing_extents() pass cxlr_dax into the scan, verify
under cxl_rwsem.region and cxl_rwsem.dpa that cxled->cxld.region ==
cxlr_dax->cxlr and cxlr->cxlr_dax == cxlr_dax, and avoid calling
cxl_release_recovered() when aborting due to a concurrent detach/unpublish?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104230445.66625-1-anisa.su@samsung.com?part=12