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

On Thu, Jun 25, 2026 at 06:20:51PM +0000, [email protected] wrote:
> 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.
True but benign. And also unlikely. Even if an entirely duplicate tag
group is ommitted from the DC add response, since the capacity was
previously added, it's still provisioned to the host until a release
event.

> - [High] Concurrent access to `group->nr_extents` without locks causes a memory leak and pins the DAX region device indefinitely.
False Positive.

> --
> 
> commit b3bfb6df5d2f0f0f9e2b022064279f74742afa2e
> Author: Ira Weiny <[email protected]>
> 
> 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?
> 
A tag group's extents are never concurrently torn down.

> 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.
>
??? An open sysfs fd does not pin the device's kobject, and closing it
does not trigger the device's release. sysfs files are backed by kernfs,
and kernfs uses active references that are taken only for the duration
of an individual read/write, then dropped — not held across an idle open
fd. The device's own kref comes from device_add() and is dropped by
device_del()/put_device() in the driver-core teardown path, not by
userspace opening or closing sysfs files.


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

Well that never happens because there is no lost decrement race.

> [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?
> 
No. cxl_add_pending() always sends a response, even if it's empty. We
simply omit fully duplicate extents from the response.

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