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 <[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? 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/[email protected]?part=16
