Re: [PATCH V3 2/9] dax/fsdev: fix multi-range offset in memory_failure handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless iteration over `dev_dax->ranges` in `fsdev_pfn_to_offset` causes a Use-After-Free.
--

commit 104845be5fb1300e103dae52129fee9336fbed14
Author: John Groves <John@Groves.net>
dax/fsdev: fix multi-range offset in memory_failure handler

This commit fixes the memory_failure offset calculation for multi-range
devices by introducing fsdev_pfn_to_offset() to walk the range list and
compute the correct device-linear byte offset.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index 188b2526bee45..f315533b299e9 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
[ ... ]
> @@ -135,11 +135,26 @@ static void fsdev_clear_ops(void *data)
>   * The core mm code in free_zone_device_folio() handles the wake_up_var()
>   * directly for this memory type.
>   */
> +static u64 fsdev_pfn_to_offset(struct dev_dax *dev_dax, unsigned long pfn)
> +{
> +	phys_addr_t phys = PFN_PHYS(pfn);
> +	u64 offset = 0;
> +
> +	for (int i = 0; i < dev_dax->nr_range; i++) {
> +		struct range *range = &dev_dax->ranges[i].range;

[Severity: High]
This is a pre-existing issue, but does iterating over dev_dax->ranges here
without locks expose us to a use-after-free?

Because fsdev_pfn_to_offset() is called asynchronously by the memory_failure
handler, it seems possible for it to race with root writing to the mapping
sysfs attribute to append ranges. That write invokes mapping_store().

Unlike size_store(), mapping_store() does not check if the DAX device is
currently bound to a driver before calling alloc_dev_dax_range(). That function
uses krealloc() to expand the dev_dax->ranges array, freeing the old memory.

If fsdev_pfn_to_offset() executes concurrently with this sysfs write, could it
dereference the freed array pointer?

Would it be safer to pass the pgmap parameter down from the memory failure
handler and iterate over pgmap->ranges instead, since it contains a stable,
immutable snapshot of the ranges created during driver probe?

> +
> +		if (phys >= range->start && phys <= range->end)
> +			return offset + (phys - range->start);
> +		offset += range_len(range);
> +	}
> +	return -1ULL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019e79caead2-5795328c-af48-4a93-b147-c11df7446e1a-000000@email.amazonses.com?part=2