Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Lockless iteration over the shared `cxlr_dax->dc_extents` xarray leads to Use-After-Free. - [High] Unsynchronized non-atomic modifications to `group->nr_extents` lead to lost decrements and tag group leak. - [High] Multi-extent tag group releases incorrectly tear down the entire group on the first chunk, causing spurious errors and WARN_ON stack traces on subsequent chunks. --
commit 277b0b7ee1e3ee9199ddbac19db51d5566770cbb Author: Ira Weiny <[email protected]> cxl/extent: Handle DC Release Capacity events This commit replaces the no-op ack stub for cxl_rm_extent() with the real teardown logic. It resolves the released DPA range to its region and endpoint decoder, locates the matching dc_extent, and tears down the entire containing tag group atomically. > diff --git a/drivers/cxl/core/extent.c b/drivers/cxl/core/extent.c > index 0ebb581ca8335..a590a89f35802 100644 > --- a/drivers/cxl/core/extent.c > +++ b/drivers/cxl/core/extent.c [ ... ] > +static void rm_tag_group(struct cxl_dc_tag_group *group) > +{ > + struct device *region_dev = &group->cxlr_dax->dev; > + struct dc_extent *dc_extent; > + unsigned long index; > + > + /* > + * Pin @group across the walk: each devm_release_action runs the > + * dc_extent_unregister action synchronously, which drops the last > + * reference on the dc_extent device and fires dc_extent_release. > + * The release decrements group->nr_extents and, on the final > + * decrement, frees @group. Without the pin the next iteration's > + * xa_find_after() dereferences a freed xarray. > + */ > + group->nr_extents++; > + xa_for_each(&group->dc_extents, index, dc_extent) > + devm_release_action(region_dev, dc_extent_unregister, > dc_extent); [Severity: High] What happens if a device releases a multi-extent tag group by sending multiple DCD release events (one per chunk)? When the host processes the first event, cxl_rm_extent() resolves the group and calls rm_tag_group(), which atomically unregisters all extents. If the sysfs reference is > 0 (e.g. userspace has a file open), the extents remain in the xarray. When subsequent release events for the remaining chunks are processed, cxl_rm_extent() finds the same group and calls rm_tag_group() again. Will invoking devm_release_action() on already-removed actions trigger a WARN_ON(!devres) stack trace? > + group->nr_extents--; > + if (!group->nr_extents) > + free_tag_group(group); [Severity: High] Can this result in lost decrements and a tag group leak? Since group->nr_extents is a plain unsigned int, a lockless decrement here in rm_tag_group() could race with dc_extent_release(). If userspace holds references to multiple extents in the same tag group via sysfs and closes them concurrently, dc_extent_release() will run on multiple CPUs simultaneously. This would race with the decrement here, preventing the counter from reaching zero and permanently leaking the cxl_dc_tag_group and its UUID. > +} > + > +int cxl_rm_extent(struct cxl_memdev_state *mds, struct cxl_extent *extent) > +{ [ ... ] > + /* > + * Find the dc_extent whose DPA range covers the released range and > + * whose tag matches. The release targets the entire containing > + * tag group atomically; partial release is not supported. > + */ > + group = NULL; > + xa_for_each(&cxlr_dax->dc_extents, idx, dc_extent) { > + if (dc_extent->cxled != cxled) [Severity: High] Is it safe to iterate the shared cxlr_dax->dc_extents xarray without RCU protection or a write lock here? cxl_rm_extent() and cxlr_dax_classify_extent() iterate over the shared xarray using xa_for_each() while holding cxl_rwsem.region only for read. If concurrent DCD events from different memdevs in the same interleaved region trigger rm_tag_group() or a failed cxl_add_extent(), dc_extent objects can be removed and synchronously freed via kfree() in dc_extent_release() by another thread. Since xa_for_each() drops the internal RCU lock during the loop body, could the iterating thread dereference a freed dc_extent pointer here? > + continue; > + if (!range_contains(&dc_extent->dpa_range, &dpa_range)) > + continue; > + if (!uuid_equal(&dc_extent->group->uuid, &tag)) > + continue; > + group = dc_extent->group; > + break; > + } > + if (!group) { > + dev_err(&cxlr_dax->dev, > + "release DPA %pra (%pU) matches no dc_extent\n", > + &dpa_range, &tag); [Severity: High] Could this error path be triggered spuriously on multi-chunk releases? If a device sends multiple DCD release events for a multi-extent tag group, the first chunk will cause rm_tag_group() to unregister all extents. If sysfs references are zero, the extents are freed immediately. When subsequent release events for the remaining chunks are processed, they will fail to find the group here and print this spurious "matches no dc_extent" error. > + return -EINVAL; > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
