Re: [PATCH v11 16/31] cxl/extent: Validate DC extent partition

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Entirely duplicate tag groups are rejected and dropped instead of replying success to the device.
- [High] Concurrent access to `group->nr_extents` without locks causes a memory leak and pins the DAX region device indefinitely.
--

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

cxl/extent: Validate DC extent partition
    
Extend cxl_validate_extent() — the per-extent check of the add pipeline
to check partition membership. Resolves an extent's DPA to its containing
DC partition. Sharability is a property of the partition (part->shareable),
taken from its CDAT DSMAS entry.

> diff --git a/drivers/cxl/core/extent.c b/drivers/cxl/core/extent.c
> index 6e67e787d14de..2e770c5279c23 100644
> --- a/drivers/cxl/core/extent.c
> +++ b/drivers/cxl/core/extent.c

[ ... ]

> @@ -101,6 +159,27 @@ static int cxl_validate_extent(struct cxl_memdev_state *mds,
>  		.end = start_dpa + le64_to_cpu(extent->length) - 1,
>  	};
>  	struct range ed_range;
> +	uuid_t uuid;
> +
> +	import_uuid(&uuid, extent->uuid);
> +
> +	part = cxl_extent_dc_partition(mds, extent, &ext_range);
> +	if (!part)
> +		return -ENXIO;
> +
> +	if (part->shareable) {
> +		if (uuid_is_null(&uuid)) {
> +			dev_err_ratelimited(dev,
> +				"DC extent DPA %pra: sharable-partition extent has null tag (firmware bug)\n",
> +				&ext_range);
> +			return -ENXIO;
> +		}
> +	} else if (le16_to_cpu(extent->shared_extn_seq)) {
> +		dev_err_ratelimited(dev,
> +			"DC extent DPA %pra (%pU): non-sharable partition but shared_extn_seq=%u (firmware bug)\n",
> +			&ext_range, &uuid, le16_to_cpu(extent->shared_extn_seq));
> +		return -ENXIO;
> +	}
>  
>  	cxlr = cxl_dpa_to_region(cxlmd, start_dpa, &cxled);
>  	if (!cxlr || !cxlr->cxlr_dax)

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition in
drivers/cxl/core/extent.c:dc_extent_release() that might lead to a leak:

	group->nr_extents--;
	if (!group->nr_extents)
		free_tag_group(group);

Could concurrent access to group->nr_extents without locks cause a lost
decrement?

When userspace closes sysfs files for multiple extents of the same tag group
concurrently, their kobject refcounts can drop to 0 simultaneously, causing
dc_extent_release() to run on multiple CPUs in parallel.

If group->nr_extents never reaches 0, does this permanently pin the DAX
region device (since the reference isn't dropped) and leak the
cxl_dc_tag_group structure?

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about how
duplicate tag groups are handled between drivers/cxl/core/extent.c and
drivers/cxl/core/mbox.c.

In drivers/cxl/core/extent.c:cxl_add_extent(), duplicate extents return 0
without allocating or assigning mds->add_ctx.group:

	case CXL_EXT_DUPLICATE:
		/*
		 * Idempotent accept simplifies the dax-side scan for existing
		 * extents on region creation; reply success without duplicating.
		 */
		dev_warn_ratelimited(&cxled->cxld.dev,
				     "Extent %pra exists; accept again\n",
				     &ext_range);
		return 0;

If all extents in a group are duplicates, cxl_realize_group() in
drivers/cxl/core/mbox.c finishes its loop with group_cnt > 0 but
mds->add_ctx.group remains NULL. It then returns -ENXIO:

	tag_group = mds->add_ctx.group;
	mds->add_ctx.group = NULL;
	if (!tag_group)
		return -ENXIO;

Since this causes the caller cxl_add_pending() to treat the group as an error
and drop the extents instead of adding them to the accepted list, does this
leave the device hanging without an ADD_DC_RESPONSE if it resends an
unacknowledged extent group?

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