Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] Retaining pointers to `dax_resource` in `dev_dax` ranges without holding a driver-core reference on the underlying extent device risks a Use-After-Free. - [High] The error rollback path in `uuid_claim_tagged()` leaks DAX mapping devres actions, leading to Use-After-Free on device teardown. - [High] The sequence validation in `uuid_claim_tagged()` fails to detect missing trailing extents, silently carving a partial allocation. - [Medium] Modifying device capacity via `uuid_store` bypasses the check for bound drivers, leading to driver state inconsistency. - [Medium] The `align` attribute is silently made read-only on dynamic DAX regions, preventing userspace from configuring huge page alignments. - [Medium] `uuid_claim_untagged()` may falsely fail with `-EINVAL` if the first discovered untagged extent is unaligned. --
commit cde0cc72c8f92d08e5dfa29000813bc5b9c53109 Author: Anisa Su <[email protected]> dax/bus: Tag-aware uuid claim and show on DC dax devices This patch introduces userspace claim semantics to resolve tagged groups into DAX devices on DC DAX regions. It populates a seed device with zero size, and allows capacity to be claimed by writing to the uuid sysfs attribute, expanding the size or consuming an untagged resource. > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > index f086ad27d5077..d94c0853af105 100644 > --- a/drivers/dax/bus.c > +++ b/drivers/dax/bus.c > @@ -1100,6 +1101,9 @@ static int alloc_dev_dax_range(struct resource *parent, > struct dev_dax *dev_dax, > }, > .dax_resource = dax_resource, > }; > + /* Pin the extent for this range; trim_dev_dax_range() drops it. */ > + if (dax_resource) > + dax_resource->use_cnt++; [Severity: High] Does this code risk a use-after-free if the underlying extent device is removed? alloc_dev_dax_range() pins the extent by incrementing use_cnt, but it does not appear to secure a reference to the underlying extent_dev via get_device(). If the parent CXL device is hot-unplugged, the core driver model will unregister and free the child extent devices and their dax_resource data. Later, when the dev_dax device is destroyed, trim_dev_dax_range() would dereference the freed dax_resource to decrement use_cnt. [ ... ] > @@ -1595,13 +1682,178 @@ static DEVICE_ATTR_RO(numa_node); > static ssize_t uuid_show(struct device *dev, > struct device_attribute *attr, char *buf) > { [ ... ] > +static ssize_t uuid_claim_untagged(struct dax_region *dax_region, > + struct dev_dax *dev_dax) > +{ > + struct dax_uuid_match match = { > + .dax_region = dax_region, > + .uuid = &uuid_null, > + }; > + struct dax_resource *dax_resource; > + resource_size_t to_alloc; > + struct device *extent_dev; > + ssize_t alloc; > + > + extent_dev = device_find_child(dax_region->dev, &match, > + find_uuid_extent); > + if (!extent_dev) > + return -ENOENT; > + > + dax_resource = dev_get_drvdata(extent_dev); > + to_alloc = resource_size(dax_resource->res); > + if (!alloc_is_aligned(dev_dax, to_alloc)) { > + put_device(extent_dev); > + return -EINVAL; > + } [Severity: Medium] Will this falsely fail if the very first untagged extent found happens to be unaligned? Since device_find_child() only returns the first matching extent, returning -EINVAL here stops the claim entirely instead of iterating to check if there is another untagged extent available that is properly aligned. [ ... ] > +static ssize_t uuid_claim_tagged(struct dax_region *dax_region, > + struct dev_dax *dev_dax, const uuid_t *uuid) > +{ [ ... ] > + /* > + * Tagged groups carry a dense 0..n-1 @seq_num regardless of source — > + * the device-stamped shared_extn_seq (already 0..n-1) for a sharable > + * partition, or cxl-side arrival order for a non-sharable one (see > + * &struct dax_resource). A gap or out-of-range value here means an > + * extent went missing on the cxl side (e.g. a per-extent failure in > + * cxl_add_pending) or a cxl-side validation gap; in either case > + * refuse the whole group rather than carve a partial allocation. > + */ > + for (i = 0; i < c.count; i++) { > + if (c.arr[i]->seq_num != i) { > + dev_WARN_ONCE(dax_region->dev, 1, > + "tag %pUb seq invariant violated at slot %u > (got %u)\n", > + uuid, i, c.arr[i]->seq_num); > + rc = -EINVAL; > + goto out; > + } > + } [Severity: High] Does this validation loop miss missing trailing extents? The loop only checks up to c.count. If a group is expected to have 4 extents, but only the first 2 (seq_num 0 and 1) were collected, the loop will successfully validate them. This seems to violate the all-or-nothing allocation invariant mentioned in the comment, silently carving a partial allocation and permanently orphaning the remaining extents since a device cannot be appended to once claimed. [ ... ] > +rollback: > + /* > + * Partial failure: trim every range we added in this attempt. > + * trim_dev_dax_range pops the most-recently-appended range from > + * dev_dax->ranges[] and decrements its dax_resource->use_cnt, so > + * looping until we have undone @i additions restores both > + * dev_dax->ranges[] and the matched dax_resources' use_cnt. > + */ > + while (i-- > 0) > + trim_dev_dax_range(dev_dax); > +out: > + kfree(c.arr); > + return rc; > +} [Severity: High] Does this error rollback leak devm mapping actions? During a successful allocation, alloc_dev_dax_range() calls devm_register_dax_mapping() which attaches an unregister_dax_mapping devres action. When trim_dev_dax_range() pops the ranges, it does not seem to call devm_release_action() to clean up the attached actions. When the dev_dax device is eventually destroyed, these leaked actions would execute and attempt to unregister mappings using array indices that no longer exist, leading to an out-of-bounds dereference on the freed dev_dax->ranges array. > static ssize_t uuid_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t len) > { [ ... ] > + ACQUIRE(rwsem_write_kill, dev_rwsem)(&dax_dev_rwsem); > + if ((rc = ACQUIRE_ERR(rwsem_write_kill, &dev_rwsem))) > + return rc; > + > + /* A claimed device already has capacity; do not overwrite its uuid. */ > + if (dev_dax_size(dev_dax)) > + return -EBUSY; [Severity: Medium] Should this also check if a driver is actively bound to the device? If userspace binds a driver to this 0-sized device before writing to the uuid attribute, this bypasses the check. Failing to check if (dev_dax->dev.driver) allows the capacity to change while a driver is already bound, which breaks the contract enforced in size_store(). [ ... ] > @@ -1661,8 +1913,12 @@ static umode_t dev_dax_visible(struct kobject *kobj, > struct attribute *a, int n) > return 0; > if (a == &dev_attr_mapping.attr && is_dynamic(dax_region)) > return 0; > - if ((a == &dev_attr_align.attr || > - a == &dev_attr_size.attr) && is_static(dax_region)) > + if (a == &dev_attr_uuid.attr && !is_dynamic(dax_region)) > + return 0444; > + if (a == &dev_attr_align.attr && > + (is_static(dax_region) || is_dynamic(dax_region))) > + return 0444; [Severity: Medium] Does this unintentionally prevent configuring alignment for dynamic regions? By forcing the align attribute to be read-only (0444) for dynamic regions, userspace cannot configure huge page alignments (like 2MB or 1GB) for DC DAX devices before they are claimed. > + if (a == &dev_attr_size.attr && is_static(dax_region)) > return 0444; > return a->mode; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=26
