Re: [PATCH v11 26/31] dax/bus: Tag-aware uuid claim and show on DC dax devices

[sashiko-bot@kernel.org]

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 <anisa.su887@gmail.com>

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/20260625112638.550691-1-anisa.su@samsung.com?part=26