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. - [High] Fully duplicate extent groups are incorrectly rejected and omitted from the device response. -- 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
