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

[sashiko-bot@kernel.org]

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.
- [High] Fully duplicate Add-Capacity groups are erroneously dropped without sending an Add-DC-Response.
- [High] Failed extents unconditionally send Release DC commands on error paths, violating the skip_device_release protocol.
- [High] Manual `xa_erase()` before `put_device()` creates a race condition that clobbers re-allocated IDs.
- [High] Concurrent teardown causes a data race on `group->nr_extents`, leading to a permanent resource leak.
- [High] Recovered extents are permanently leaked to the device if their individual tracking allocation fails.
- [High] Tag groups spanning multiple DAX regions are silently grouped under the first region, corrupting HPA ranges.
--

commit c1287602ad57e47bb90d02808a1f908fcb287a89
Author: Ira Weiny <iweiny@kernel.org>
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?

> +	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?

> +			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?

> +
> +	/*
[ ... ]
> +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?

[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?

> +
> +		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?

> +		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?

> +
> +	rc = online_tag_group(tag_group, !existing);

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