Re: [PATCH V3 5/9] dax/fsdev: use __va(phys) for kaddr in direct_access

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch leaves behind dead code for calculating `data_offset` and assigning `dev_dax->virt_addr` in `fsdev_dax_probe()`.

Pre-existing issues:
- [High] `__fsdev_dax_direct_access()` returns `-EFAULT` when the requested `size` crosses a physical range boundary, instead of truncating the return value to the available contiguous pages.
--

commit b771878942b230ec4df1330dccd98b1983cf30b8
Author: John Groves <John@Groves.net>

dax/fsdev: use __va(phys) for kaddr in direct_access

This commit fixes the kernel virtual address calculation in
__fsdev_dax_direct_access() by using __va(phys) instead of a linear
offset from virt_addr. This ensures correct behavior for multi-range
devices with physical gaps.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index 42aac7e952516..a2d2eb20fb4d0 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
> @@ -51,9 +51,7 @@ 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;
> -	void *virt_addr = dev_dax->virt_addr + offset;

[Severity: Low]
By removing this calculation, is virt_addr now entirely unused?

Looking at the rest of the driver, dev_dax->virt_addr doesn't appear to be
read anywhere else. Should the virt_addr field in struct dev_dax and its
assignment in fsdev_dax_probe() be removed to clean up the dead code?

>  	phys_addr_t phys;
> -	unsigned long local_pfn;
>  
>  	phys = dax_pgoff_to_phys(dev_dax, pgoff, size);
>  	if (phys == -1) {
>  		dev_dbg(&dev_dax->dev,
>  			"pgoff (%#lx) out of range\n", pgoff);
>  		return -EFAULT;
>  	}

[Severity: High]
This is a pre-existing issue, but does returning -EFAULT here break the
direct_access API contract when a request crosses a physical range boundary?

If the requested size extends beyond the current physical range on a
multi-range device, dax_pgoff_to_phys() returns -1, causing this to fail
entirely. Should this instead truncate the return value to the available
physically contiguous pages rather than failing the whole transfer?

Additionally, the fallback size calculation later in this function:

return PHYS_PFN(min(size, dev_dax->cached_size - offset));

seems to incorrectly assume physical contiguity across range gaps.

>  
>  	if (kaddr)
> -		*kaddr = virt_addr;
> +		*kaddr = __va(phys);
>  
> -	local_pfn = PHYS_PFN(phys);
>  	if (pfn)
> -		*pfn = local_pfn;
> +		*pfn = PHYS_PFN(phys);
>  
>  	/*
>  	 * Use cached_size which was computed at probe time. The size cannot

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