Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Double `xa_erase` race condition in `online_tag_group` error path can silently delete concurrently allocated extents. - [High] Partial group accepted in `cxl_realize_group` on `cxl_add_extent` failure, violating More-chain atomicity. -- commit 802d981e0cb1d1c5a24f7358b9d9626230786651 Author: Ira Weiny <[email protected]> cxl/region/extent: Expose dc_extent information in sysfs Extent information can be helpful to the user to coordinate memory usage with the external orchestrator and FM. Expose the details of each dc_extent by creating sysfs entries. > diff --git a/drivers/cxl/core/extent.c b/drivers/cxl/core/extent.c > index 36be56ca10974..69c993cdd558b 100644 > --- a/drivers/cxl/core/extent.c > +++ b/drivers/cxl/core/extent.c [ ... ] > @@ -99,6 +154,7 @@ static void dc_extent_release(struct device *dev) > static const struct device_type dc_extent_type = { > .name = "extent", > .release = dc_extent_release, > + .groups = dc_extent_attribute_groups, > }; [Severity: High] This is a pre-existing issue, but does the error path in online_tag_group() have a race condition that could silently delete concurrently allocated extents? If dev_set_name() or device_add() fails in online_tag_group(), the error path calls xa_erase() followed by put_device(): drivers/cxl/core/extent.c:online_tag_group() { ... rc = dev_set_name(dev, "extent%d.%d", cxlr_dax->cxlr->id, dev->id); if (rc) { xa_erase(&cxlr_dax->dc_extents, dev->id); put_device(dev); break; } rc = device_add(dev); if (rc) { xa_erase(&cxlr_dax->dc_extents, dev->id); put_device(dev); break; } ... } However, put_device() synchronously invokes dc_extent_release(), which unconditionally executes a second xa_erase() for the same id: drivers/cxl/core/extent.c:dc_extent_release() { ... group = dc_extent->group; cxled_release_extent(dc_extent->cxled, dc_extent); xa_erase(&group->cxlr_dax->dc_extents, dc_extent->dev.id); ... } Since the xarray is configured with XA_FLAGS_ALLOC1, a concurrent thread processing another DCD event could allocate this id via xa_alloc() immediately after the first xa_erase(). Could the second xa_erase() in the release function erroneously delete the concurrent thread's newly allocated extent, breaking region capacity accounting? [Severity: High] This is a pre-existing issue, but does the error handling in cxl_realize_group() allow partial tag groups to be onlined? If cxl_add_extent() fails for any extent in a More-chain group (e.g., due to an overlap or memory allocation failure), the loop calls delete_extent_node() and executes continue: drivers/cxl/core/mbox.c:cxl_realize_group() { ... if (cxl_add_extent(mds, pos->extent, seq_num, existing) < 0) { dev_dbg(dev, "Tag %pUb: failed to add extent DPA:%#llx LEN:%#llx\n", tag, le64_to_cpu(pos->extent->start_dpa), le64_to_cpu(pos->extent->length)); delete_extent_node(pos); continue; } group_cnt++; ... } By using continue, it bypasses the failure and leaves the remaining valid extents in the group, resulting in a partial tag group being successfully onlined. Could this cause the host to surface a broken or incomplete DAX resource, violating the architectural intent to either accept or reject tagged allocations atomically? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=20
