Re: [PATCH v11 14/31] cxl/extent: Handle DC Add Capacity events

On Thu, Jun 25, 2026 at 06:16:35PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> - [High] Use-after-free in `cxlr_dax_classify_extent` due to lockless XArray iteration.
Real -- fixed with RCU-deferred free + rcu_read_lock()

> - [High] Fully duplicate Add-Capacity groups are erroneously dropped without sending an Add-DC-Response.
False Positive -- 1 response is always sent for each chain even if
empty.

> - [High] Failed extents unconditionally send Release DC commands on error paths, violating the skip_device_release protocol.
Real -- fixed with a minor caveat.

> - [High] Manual `xa_erase()` before `put_device()` creates a race condition that clobbers re-allocated IDs.
Real -- drop redundant manual xa_erase().

> - [High] Concurrent teardown causes a data race on `group->nr_extents`, leading to a permanent resource leak.
False Positive -- teardown is never concurrent. group->nr_extents only
released serially.

> - [High] Recovered extents are permanently leaked to the device if their individual tracking allocation fails.
False Positive -- Rcovered extents ARE released to device on failure.

> - [High] Tag groups spanning multiple DAX regions are silently grouped under the first region, corrupting HPA ranges.
False Positive -- subsequent patches robustify partition and tag-group
semantics; tag groups spanning multiple partitions are rejected by
cxl_check_group_partition().

> --
> 
> commit c1287602ad57e47bb90d02808a1f908fcb287a89
> Author: Ira Weiny <[email protected]>
> Subject: cxl/extent: Handle DC Add Capacity events
> 
> Replace the empty-response stub in handle_add_event() with the real
> add pipeline. This commit fills in the logic for processing the pending list
> and adds basic validation for extents before they are added to the
> device model as a child of the cxlr_dax region.
> 
> > diff --git a/drivers/cxl/core/extent.c b/drivers/cxl/core/extent.c
> > new file mode 100644
> > index 0000000000000..6e67e787d14de
> > --- /dev/null
> > +++ b/drivers/cxl/core/extent.c
> > @@ -0,0 +1,411 @@
> [ ... ]
> > +static void dc_extent_release(struct device *dev)
> > +{
> > +	struct dc_extent *dc_extent = to_dc_extent(dev);
> > +	struct cxl_dc_tag_group *group;
> > +
> > +	if (!dc_extent)
> > +		return;
> > +
> > +	group = dc_extent->group;
> > +	cxled_release_extent(dc_extent->cxled, dc_extent);
> > +	xa_erase(&group->cxlr_dax->dc_extents, dc_extent->dev.id);
> > +	xa_erase(&group->dc_extents, dc_extent->seq_num);
> > +	group->nr_extents--;
> > +	if (!group->nr_extents)
> > +		free_tag_group(group);
> 
> [Severity: High]
> Is it safe to decrement group->nr_extents without a lock? Since multiple
> extents in the same group can be released concurrently, couldn't this
> non-atomic decrement cause a data race, leading to a permanent memory leak
> of the cxl_dc_tag_group and its pinned cxlr_dax->dev reference?
> 
A tag group is torn down by a single memdev's event
thread so the tag group's extents are released serially: either through
the release event or during teardown under cxl_rwsem.region held for
write.

> > +	kfree(dc_extent);
> > +}
> [ ... ]
> > +static enum cxl_extent_class
> > +cxlr_dax_classify_extent(struct cxl_dax_region *cxlr_dax,
> > +			 struct cxl_endpoint_decoder *cxled,
> > +			 const struct range *ext_range)
> > +{
> > +	struct dc_extent *entry;
> > +	unsigned long i;
> > +
> > +	xa_for_each(&cxlr_dax->dc_extents, i, entry) {
> > +		if (entry->cxled != cxled)
> 
> [Severity: High]
> Is this iteration safe against concurrent teardown? xa_for_each drops the
> internal RCU read lock between iterations, leaving the returned entry
> unprotected. If online_tag_group fails concurrently or there is a
> parallel release, couldn't accessing entry->cxled trigger a use-after-free?
> 
Correct. cxlr_dax->dc_extents is per-region and shared across
memdevs, whereas log_lock is per-memdev, so two memdevs' event threads can
walk and free the xarray concurrently while each holds only cxl_rwsem.region
for read. Note that this only applies to interleaved regions spanning >=
2 memdevs. Single memdev regions were protected by the memdev's
log_lock.

The fix is to make the free RCU-deferred.
- rcu_head is added to struct dc_extent
- we call kfree_rcu() in dc_extent_release()
- guard(rcu) called before xa_for_each() in this function
- Patch 18 introduces cxl_rm_extent(), which also walks
  cxlr_dax->dc_extents to find the right extent to release. This also
  needs guard(rcu)
  	- specifically, it calls scoped_guard(rcu){} to unlock before
	the call to cxl_region_invalidate_memregion() which can sleep. 

> > +			continue;
> > +		if (range_contains(&entry->dpa_range, ext_range))
> > +			return CXL_EXT_DUPLICATE;
> [ ... ]
> > +static int cxlr_add_extent(struct cxl_memdev_state *mds,
> > +			   struct cxl_dax_region *cxlr_dax,
> > +			   struct dc_extent *dc_extent)
> > +{
> > +	struct cxl_dc_tag_group **group = &mds->add_ctx.group;
> > +	int rc;
> > +
> > +	if (*group && !uuid_equal(&(*group)->uuid, &dc_extent->uuid)) {
> > +		kfree(dc_extent);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!*group) {
> > +		dev_dbg(&cxlr_dax->dev, "Alloc new tag group\n");
> > +		*group = alloc_tag_group(cxlr_dax, &dc_extent->uuid);
> > +		if (IS_ERR(*group)) {
> > +			rc = PTR_ERR(*group);
> > +			*group = NULL;
> > +			kfree(dc_extent);
> > +			return rc;
> > +		}
> > +	} else {
> > +		dev_dbg(&cxlr_dax->dev, "Append dc_extent to tag group\n");
> > +	}
> > +
> > +	dc_extent->group = *group;
> 
> [Severity: High]
> What happens if extents in the same tag group (sharing a UUID) map to
> different endpoint decoders and thus different DAX regions? This logic
> appears to allocate the tag group using the cxlr_dax of the first extent
> and blindly links subsequent extents to it. Could this cause cross-region
> extents to be parented to the wrong region and corrupt their HPA mapping
> offsets?
> 
False Positive.

Subsequent patches "cxl/mem: Enforce tag-group semantics" and
"cxl/extent: Validate DC extent partition", reject a tagged group
spanning multiple partitions via cxl_check_group_partition()

> > +
> > +	/*
> [ ... ]
> > +int online_tag_group(struct cxl_dc_tag_group *group, bool skip_release)
> > +{
> [ ... ]
> > +		rc = dev_set_name(dev, "extent%d.%d", cxlr_dax->cxlr->id,
> > +				  dev->id);
> > +		if (rc) {
> > +			xa_erase(&cxlr_dax->dc_extents, dev->id);
> > +			put_device(dev);
> > +			break;
> > +		}
> > +
> > +		rc = device_add(dev);
> > +		if (rc) {
> > +			xa_erase(&cxlr_dax->dc_extents, dev->id);
> > +			put_device(dev);
> > +			break;
> > +		}
> 
> [Severity: High]
> Does the manual xa_erase() before put_device() create a double-erase race
> condition?
> 
> put_device(dev) synchronously calls dc_extent_release(), which performs a
> second xa_erase() for the same ID. If a concurrent thread allocates an ID
> between the first and second erase, wouldn't the second erase silently
> clobber the new valid allocation?
> 
Yes, for regions interleaved across >= memdevs, so it can possibly
have 2 event threads handling a DC add event concurrently.

The manual xa_erase in both error paths here are deleted.

> [Severity: High]
> Additionally, does this error path violate the skip_device_release protocol?
> At this point, group->skip_device_release has not been set yet. Because it
> is false, won't dc_extent_release() unconditionally send a Release DC command
> to the hardware for capacity the host never formally accepted?
> 
Fixed.

Setting/clearing skip_device_release is managed by the caller --
cxl_realize_group().

So in cxl_realize_group():

tag_group->skip_device_release = !existing;   // (1) set before onlining
rc = online_tag_group(tag_group);	// err paths honor flag
  ...
rc = cxlr_notify_extent(...DCD_ADD_CAPACITY...); 
if (rc) { rm_tag_group(tag_group); return rc; }    // honors flag

tag_group->skip_device_release = false;       // (2) clear after notifying dax layer


Since skip_device_release is managed by the caller, remove the skip_release
param from this function.


Caveat: in cxl_add_pending(): cxl_realize_group() is called right before
cxl_send_dc_response(), but the fully "correct" behavior would be to set
it only after cxl_send_dc_response() returns 0.

cxl_add_pending(...) {
	while(pending_list !empty) {
		extract_tag_group()

		cnt = cxl_realize_group(tag_group) <--- set/clear skip_device_release
		if (cnt < 0) {
			drop_extent_group(&group);
			continue;
		}

		list_splice_tail_init(&group, &accepted);
		total_accepted += cnt;
	}

	list_splice(&accepted, pending);

	/*
	 * Recovered (already-accepted) extents must not be re-reported in an
	 * Add-DC-Response: the device rejects a DPA range already added by a
	 * previous response (CXL r4.0 8.2.10.9.9.3, Invalid Physical Address).
	 */
	if (existing)
		return 0;

	return cxl_send_dc_response(mds, CXL_MBOX_OP_ADD_DC_RESPONSE,
				    pending, total_accepted);
}

This is a best-effort fix. Outside of the while loop, we don't have a
reference to the tag_groups, and would need to re-extract every tag
group in order to set the flag after cxl_send_dc_response. Additionally, nothing between
cxl_realize_group() and cxl_send_dc_response() affects the
skip_device_release flag, so the gap is small. The device also simply
rejects spurious Release responses with Invalid Physical Address, which
is benign.


The "true" fix would be to modify cxl_send_dc_response to extract
all tag_groups from the extent list (pending).

Then in each error scenario:

1. Fail to alloc response:

struct cxl_mbox_dc_response *response __free(kfree) =
						kzalloc(pl_size, GFP_KERNEL);
if (!response) {

	*** set each tag group's skip_device_release = true ***

	/* Refuse all extents rather than stall the device. */
	struct cxl_mbox_dc_response empty = { };

	return send_one_response(cxl_mbox, &empty, opcode, 0, 0);
}

2. If the extent list is too long and must be split across multiple
responses, we further have to track which tag groups were successfully
accepted or even partially accepted, since the response orders extents
in arrival order, and are not necessarily grouped by tag.

IMO the additional complexity to handle these edge cases is not
justifiable.


> > +
> > +		dev_dbg(dev, "dc_extent HPA %pra (%pU)\n",
> [ ... ]
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 4e887b5cdc3ed..08f51b8807c06 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> [ ... ]
> > +static int cxl_realize_group(struct cxl_memdev_state *mds, const uuid_t *tag,
> > +			     struct list_head *group, bool existing)
> > +{
> > +	struct device *dev = mds->cxlds.dev;
> > +	struct cxl_extent_list_node *pos, *tmp;
> > +	struct cxl_dc_tag_group *tag_group;
> > +	int group_cnt = 0;
> > +	int rc;
> > +
> > +	list_for_each_entry_safe(pos, tmp, group, list) {
> > +		/*
> > +		 * Pass the device-stamped 0-based shared_extn_seq through
> > +		 * unchanged as the dax-side @seq_num (0..n-1).
> > +		 */
> > +		u16 seq = le16_to_cpu(pos->extent->shared_extn_seq);
> > +
> > +		if (cxl_add_extent(mds, pos->extent, seq) < 0) {
> > +			dev_dbg(dev,
> > +				"Tag %pUb: failed to add extent DPA:%#llx LEN:%#llx\n",
> > +				tag,
> > +				le64_to_cpu(pos->extent->start_dpa),
> > +				le64_to_cpu(pos->extent->length));
> > +			delete_extent_node(pos);
> > +			continue;
> > +		}
> 
> [Severity: High]
> If cxl_add_extent() fails during initialization of existing extents
> (e.g., under memory pressure), we simply delete the node and continue.
> Because the extent is never added to a tag group, the teardown error path
> cannot see it. Does this permanently strand the capacity on the device since
> no Release DC command is sent?
> 
False Positive.
Existing extents never set skip_device_release so on failure, they are
released back to the device.

The skip_device_release flags and logic only applies to fresh extents.
> > +		group_cnt++;
> > +	}
> > +
> > +	tag_group = mds->add_ctx.group;
> > +	mds->add_ctx.group = NULL;
> > +	if (!tag_group)
> > +		return -ENXIO;
> 
> [Severity: High]
> If an Add Capacity event contains only extents that the host has already
> accepted (duplicates), cxl_add_extent() succeeds but returns 0, and
> mds->add_ctx.group is never allocated. Does this erroneously fail the
> entire group with -ENXIO, causing cxl_add_pending() to drop it without
> sending an Add-DC-Response, thereby stalling the device?
> 
False Positive. 

cxl_add_pending() unconditionally calls cxl_send_dc_response() at the
end. If every extent in a group is a duplicate, an empty response is
sent.

-ENXIO is returned here and in cxl_add_pending():

while(!list_empty(pending)){

	cnt = cxl_realize_group(mds, &tag, &group, shareable, existing);
	if (cnt < 0) {
		drop_extent_group(&group);
		continue;
	}
		list_splice_tail_init(&group, &accepted);

	total_accepted += cnt;
}
list_splice(&accepted, pending);

/*
 * Recovered (already-accepted) extents must not be re-reported in an
 * Add-DC-Response: the device rejects a DPA range already added by a
 * previous response (CXL r4.0 8.2.10.9.9.3, Invalid Physical Address).
 */
if (existing)
	return 0;

return cxl_send_dc_response(mds, CXL_MBOX_OP_ADD_DC_RESPONSE,
				    pending, total_accepted);


^total_accepted is never incremented so the response is empty.

> > +
> > +	rc = online_tag_group(tag_group, !existing);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14