Re: [PATCH V2 4/7] dax/fsdev: clamp direct_access return to current physical range

[Dave Jiang <dave.jiang@intel.com>]

On 5/22/26 12:19 PM, John Groves wrote:
> From: John Groves <John@Groves.net>
> 
> __fsdev_dax_direct_access() returned the number of available pages based
> on cached_size (the total size across all ranges). For multi-range
> devices with physical gaps between ranges, this over-reports the number
> of physically contiguous pages available from the returned kaddr/pfn.
> Callers trust this return value to mean contiguous pages, so accessing
> beyond the current range boundary would hit unmapped or unrelated memory.
> 
> Fix by finding the range that contains the translated physical address
> and clamping the return to the remaining pages within that range.
> 
> Also remove the now-unused cached_size field from struct dev_dax, since
> it was only consumed by the old return calculation.
> 
> Fixes: 099c81a1f0ab3 ("dax: Add dax_operations for use by fs-dax on fsdev dax")
> Signed-off-by: John Groves <john@groves.net>

I ran this through Claude and this is what it came back with and it looks reasonable to me:

The claimed bug does not manifest in the current tree. This is a
  refactor + dead-field removal dressed as a bugfix. Either the justification is
   wrong or there's a missing companion change.

  The claim doesn't hold up

  Commit message:

  ▎ __fsdev_dax_direct_access() returned the number of available pages based on
  ▎ cached_size (the total size across all ranges). For multi-range devices with
  ▎  physical gaps between ranges, this over-reports the number of physically
  ▎ contiguous pages available from the returned kaddr/pfn.

  Walk through it. __fsdev_dax_direct_access does:

  phys = dax_pgoff_to_phys(dev_dax, pgoff, size);
  if (phys == -1)
      return -EFAULT;
  ...
  return PHYS_PFN(min(size, dev_dax->cached_size - offset));

  Now look at what dax_pgoff_to_phys (drivers/dax/bus.c:1433) enforces:

  phys = PFN_PHYS(pgoff - dax_range->pgoff) + range->start;
  if (phys + size - 1 <= range->end)
      return phys;
  break;        /* falls through to return -1 */

  It returns valid phys only when the entire requested size fits inside a single
   range. If the request straddles a gap, it returns -1 and the caller bails
  with -EFAULT before ever reaching the clamp.

  So in the only path where the clamp runs, we already know size <= range->end -
   phys + 1, which means cached_size - offset >= size, which means min(size,
  cached_size - offset) == size. Pre-patch returns nr_pages. Post-patch returns
  nr_pages. Same answer.

  I worked it through with concrete numbers on a 2-range device with a physical
  gap (range[0]=4 pages at 0x1000_0000, range[1]=4 pages at 0x2000_0000):
  - pgoff=2, nr_pages=2: both return 2. ✓
  - pgoff=3, nr_pages=2 (straddles gap): both return -EFAULT via the early bail.
   ✓
  - pgoff=4, nr_pages=2: both return 2. ✓

  I cannot construct a case where pre-patch over-reports.

  What's actually changed

  1. A no-op clamp loop replaces a constant calculation. Same return value in
  every reachable case.
  2. The trailing return -EFAULT; is dead code. If dax_pgoff_to_phys returned
  valid phys, then by construction phys lies inside some range in
  dev_dax->ranges[], so the loop will match.
  3. O(N) walk replaces O(1) arithmetic. Trivial for small nr_range, but it's a
  regression in the wrong direction for a hot path.
  4. cached_size field is removed. Good cleanup — I grepped, no other consumers.


DJ

> ---
>  drivers/dax/dax-private.h |  2 --
>  drivers/dax/fsdev.c       | 23 ++++++++++++++---------
>  2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h
> index 81e4af49e39c1..7a3727d76a68a 100644
> --- a/drivers/dax/dax-private.h
> +++ b/drivers/dax/dax-private.h
> @@ -70,7 +70,6 @@ struct dev_dax_range {
>   * @region: parent region
>   * @dax_dev: core dax functionality
>   * @virt_addr: kva from memremap; used by fsdev_dax
> - * @cached_size: size of daxdev cached by fsdev_dax
>   * @align: alignment of this instance
>   * @target_node: effective numa node if dev_dax memory range is onlined
>   * @dyn_id: is this a dynamic or statically created instance
> @@ -86,7 +85,6 @@ struct dev_dax {
>  	struct dax_region *region;
>  	struct dax_device *dax_dev;
>  	void *virt_addr;
> -	u64 cached_size;
>  	unsigned int align;
>  	int target_node;
>  	bool dyn_id;
> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index aac0130ab2833..f74fd1bb7f4c5 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -50,8 +50,8 @@ static long __fsdev_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff,
>  {
>  	struct dev_dax *dev_dax = dax_get_private(dax_dev);
>  	size_t size = nr_pages << PAGE_SHIFT;
> -	size_t offset = pgoff << PAGE_SHIFT;
>  	phys_addr_t phys;
> +	int i;
>  
>  	phys = dax_pgoff_to_phys(dev_dax, pgoff, size);
>  	if (phys == -1) {
> @@ -67,10 +67,20 @@ static long __fsdev_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff,
>  		*pfn = PHYS_PFN(phys);
>  
>  	/*
> -	 * Use cached_size which was computed at probe time. The size cannot
> -	 * change while the driver is bound (resize returns -EBUSY).
> +	 * Return the number of physically contiguous pages available from
> +	 * phys, clamped to the current range. For multi-range devices the
> +	 * ranges may not be physically contiguous, so we cannot report
> +	 * pages beyond the end of the range that contains phys.
>  	 */
> -	return PHYS_PFN(min(size, dev_dax->cached_size - offset));
> +	for (i = 0; i < dev_dax->nr_range; i++) {
> +		struct range *range = &dev_dax->ranges[i].range;
> +
> +		if (phys >= range->start && phys <= range->end)
> +			return PHYS_PFN(min(size,
> +					    (size_t)(range->end - phys + 1)));
> +	}
> +
> +	return -EFAULT;
>  }
>  
>  static int fsdev_dax_zero_page_range(struct dax_device *dax_dev,
> @@ -272,11 +282,6 @@ static int fsdev_dax_probe(struct dev_dax *dev_dax)
>  		}
>  	}
>  
> -	/* Cache size now; it cannot change while driver is bound */
> -	dev_dax->cached_size = 0;
> -	for (i = 0; i < dev_dax->nr_range; i++)
> -		dev_dax->cached_size += range_len(&dev_dax->ranges[i].range);
> -
>  	/*
>  	 * Use MEMORY_DEVICE_FS_DAX without setting vmemmap_shift, leaving
>  	 * folios at order-0. Unlike device.c (MEMORY_DEVICE_GENERIC), this