Re: [PATCH v11 17/31] cxl/mem: Enforce tag-group semantics

On Thu, Jun 25, 2026 at 06:24:18PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Untagged extents spanning different DC partitions and DAX regions are incorrectly coalesced and assigned to the same region.

Fixed in patch 12. extract_tag_group() originally put all untagged
extents into the same tag_group. This has been fixed to put each
untagged extent into its own tag_group so they are not coalesced and
assigned to the same region.

> - [High] Fully duplicate extent groups are incorrectly rejected and omitted from the device response.

Duplicate false positive from patch 14 and 16.
> --
> 
> commit c2cf18ae81b1c32953f39b06aeaa9e153f4ac609
> Author: Anisa Su <[email protected]>
> 
> cxl/mem: Enforce tag-group semantics
> 
> This commit completes tag-group validation. It adds sequence integrity
> checks to verify that sharable partitions have dense shared_extn_seq
> values, and verifies that tagged allocations do not span DC partitions.
> 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 14ba263044f08..7967b0db2c512 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> 
> [ ... ]
> 
> > +static int cxl_check_group_partition(struct cxl_memdev_state *mds,
> > +				     const uuid_t *tag,
> > +				     const struct list_head *group)
> > +{
> > +	struct device *dev = mds->cxlds.dev;
> > +	const struct cxl_dpa_partition *first_part = NULL;
> > +	u64 first_dpa = 0;
> > +	struct cxl_extent_list_node *pos;
> > +
> > +	if (uuid_is_null(tag) || list_empty(group))
> > +		return 0;
> 
> [Severity: High]
> This is a pre-existing issue, but does this explicit bypass for untagged
> extents allow them to span different DC partitions?
> 
> If a device sends multiple untagged DC add capacity events spanning
> different partitions, cxl_add_pending() appears to extract them all into a
> single group.
> 
> Since this check returns 0 for null UUIDs, cxl_realize_group() processes them
> together. Would cxlr_add_extent() then register them all under the DAX region
> of the first extent, effectively merging extents from different partitions into
> the same region's resource tree?
> 
> [ ... ]
> 
> >  static int cxl_realize_group(struct cxl_memdev_state *mds, const uuid_t *tag,
> > -			     struct list_head *group, bool existing)
> > +			     struct list_head *group, bool shareable,
> > +			     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;
> > +	u16 seq_num;
> >  	int rc;
> >  
> > +	seq_num = -1;
> >  	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 (shareable)
> > +			seq_num = le16_to_cpu(pos->extent->shared_extn_seq);
> > +		else
> > +			seq_num++;
> >  
> > -		if (cxl_add_extent(mds, pos->extent, seq) < 0) {
> > +		if (cxl_add_extent(mds, pos->extent, seq_num) < 0) {
> 
> [Severity: High]
> This is a pre-existing issue, but what happens if a device resends a tag group
> where all extents have already been accepted?
> 
> If cxl_add_extent() detects a duplicate, it returns 0 without calling
> cxlr_add_extent(). If this happens for every extent in the group,
> mds->add_ctx.group is never allocated and remains NULL.
> 
> Does this cause the subsequent if (!tag_group) check in this function to drop
> the group and return -ENXIO instead of returning group_cnt to acknowledge
> the extents back to the device?
> 
> >  			dev_dbg(dev,
> >  				"Tag %pUb: failed to add extent DPA:%#llx LEN:%#llx\n",
> >  				tag,
> > @@ -1412,15 +1532,22 @@ static int cxl_realize_group(struct cxl_memdev_state *mds, const uuid_t *tag,
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=17