Re: [PATCH 4/4] uio: implement huge_fault support for physical mappings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] uio: BUG_ON in uio_physical_huge_fault() on COW physical
  mappings
- [Medium] uio: read-only PTE after fixup_user_fault() in
  uio_physical_access()
--

commit 64a7c2da3be318c1019224cd530d155344992a33
Author: Eliav Farber <farbere@amazon.com>

uio: implement huge_fault support for physical mappings

This commit populates non-COW UIO physical mappings on fault via .fault and
.huge_fault handlers using vmf_insert_pfn_order(), while tracking the PFN
range with pfn_range_track() and faulting pages in from .access.

> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index f8fa205226601..6b4c79340e5c8 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -733,9 +733,80 @@ static int uio_mmap_logical(struct vm_area_struct *vma)
>  	return 0;
>  }
>  
> +/*
> + * Populate UIO physical mappings on fault, using a PMD or PUD mapping where the
> + * architecture, the fault address and the pfn allow it.
> + */
> +static vm_fault_t uio_physical_huge_fault(struct vm_fault *vmf,
> +					  unsigned int order)
> +{
> +	struct vm_area_struct *vma = vmf->vma;
> +	unsigned long addr = vmf->address & ~((PAGE_SIZE << order) - 1);
> +	struct uio_device *idev = vma->vm_private_data;
> +	vm_fault_t ret = VM_FAULT_SIGBUS;
> +	unsigned long pfn;
> +
> +	/*
> +	 * vm_pgoff is the base pfn of the mapping (see uio_mmap_physical()).
> +	 * vmf_pfn_order_ok() ignores pfn if the window starts below vm_start.
> +	 */
> +	pfn = vma->vm_pgoff + ((addr - vma->vm_start) >> PAGE_SHIFT);
> +	if (!vmf_pfn_order_ok(vmf, pfn, order))
> +		return VM_FAULT_FALLBACK;
> +
> +	mutex_lock(&idev->info_lock);
> +
> +	if (idev->info)
> +		ret = vmf_insert_pfn_order(vmf, pfn, order);
> +
> +	mutex_unlock(&idev->info_lock);
> +
> +	return ret;
> +}
> +
> +static vm_fault_t uio_physical_fault(struct vm_fault *vmf)
> +{
> +	return uio_physical_huge_fault(vmf, 0);
> +}
> +
> +#ifdef CONFIG_HAVE_IOREMAP_PROT
> +/*
> + * generic_access_phys() only follows present PTEs, and a lazily populated
> + * mapping has none for pages the target has not touched yet. Fault the page in
> + * first (through the handlers above, so info_lock is honoured), so that remote
> + * access (ptrace, /proc/pid/mem) keeps working.
> + *
> + * fixup_user_fault() refuses a fault the vma does not permit, so an untouched
> + * page of a mapping without VM_READ stays unreadable, where the eager mapping
> + * left a present PTE.
> + */
> +static int uio_physical_access(struct vm_area_struct *vma, unsigned long addr,
> +			       void *buf, int len, int write)
> +{
> +	unsigned int fault_flags = FAULT_FLAG_REMOTE;
> +
> +	if (write)
> +		fault_flags |= FAULT_FLAG_WRITE;
> +
> +	/*
> +	 * COW mappings are populated at mmap() time, and a write fault would
> +	 * copy the page instead. With unlocked == NULL, fixup_user_fault()
> +	 * keeps the mmap lock held and the vma valid.
> +	 */
> +	if (!vma_is_cow_mapping(vma))
> +		fixup_user_fault(vma->vm_mm, addr, fault_flags, NULL);
> +
> +	return generic_access_phys(vma, addr, buf, len, write);
> +}
> +#endif

[Severity: Medium]
Can a remote write to an untouched page fail with -EINVAL after soft-dirty
bits are cleared via /proc/<pid>/clear_refs?

Writing CLEAR_REFS_SOFT_DIRTY to /proc/<pid>/clear_refs clears VM_SOFTDIRTY on
the VMA and calls vma_set_page_prot(), which strips write permission from
vma->vm_page_prot so soft-dirty tracking can catch writes.  When
fixup_user_fault() then faults an unmapped page at order 0,
vmf_insert_pfn_order() calls vmf_insert_pfn(), which passes mkwrite = false to
insert_pfn() and installs a read-only PTE from vma->vm_page_prot, returning
VM_FAULT_NOPAGE without running wp_pfn_shared().  generic_access_phys() then
sees !args.writable and rejects the FOLL_WRITE access with -EINVAL.

> +
>  static const struct vm_operations_struct uio_physical_vm_ops = {
>  #ifdef CONFIG_HAVE_IOREMAP_PROT
> -	.access = generic_access_phys,
> +	.access = uio_physical_access,
> +#endif
> +	.fault = uio_physical_fault,
> +#ifdef CONFIG_ARCH_SUPPORTS_HUGE_PFNMAP
> +	.huge_fault = uio_physical_huge_fault,
>  #endif
>  };
>  
> @@ -767,11 +838,30 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
>  	 * So we just do the physical mmap without a page
>  	 * offset.
>  	 */
> -	return remap_pfn_range(vma,
> -			       vma->vm_start,
> -			       mem->addr >> PAGE_SHIFT,
> -			       vma->vm_end - vma->vm_start,
> -			       vma->vm_page_prot);
> +	if (vma_is_cow_mapping(vma)) {
> +		/* COW mappings cannot be populated on fault: map them now. */
> +		return remap_pfn_range(vma,
> +				       vma->vm_start,
> +				       mem->addr >> PAGE_SHIFT,
> +				       vma->vm_end - vma->vm_start,
> +				       vma->vm_page_prot);
> +	}

[Severity: High]
Can installing uio_physical_vm_ops on a COW mapping trigger a kernel BUG_ON()
when a concurrent fault observes a transiently cleared PTE?

uio_mmap_physical() sets vma->vm_ops = &uio_physical_vm_ops before checking
vma_is_cow_mapping(vma), so a MAP_PRIVATE writable VMA gets .fault and
.huge_fault installed even though it is mapped eagerly by remap_pfn_range():

CPU0                                  CPU1
handle_pte_fault()
  do_wp_page()
    wp_page_copy()
      ptep_clear_flush(...)
                                      handle_pte_fault()
                                        orig_pte = ptep_get_lockless(...)
                                        /* sees pte_none(orig_pte) */
                                        do_pte_missing()
                                          do_fault()
                                            do_read_fault()
                                              __do_fault()
                                                uio_physical_fault()
                                                  vmf_insert_pfn_order()
                                                    vmf_insert_pfn_prot()
                                                      BUG_ON(...)
      set_pte_at(...)

Previously .fault was NULL, so do_fault() took the !vma->vm_ops->fault branch,
rechecked the PTE under the page table lock, and returned VM_FAULT_NOPAGE.
With .fault set and no vma_is_cow_mapping() check in uio_physical_huge_fault(),
do_fault() calls into vmf_insert_pfn_prot() (or the PMD/PUD equivalents),
which hits BUG_ON((vma->vm_flags & VM_PFNMAP) && vma_is_cow_mapping(vma)).

Even if uio_physical_huge_fault() checked vma_is_cow_mapping() and returned
VM_FAULT_SIGBUS, having .fault set on a COW VMA would still bypass do_fault()'s
page table lock recheck and deliver a spurious SIGBUS on transient PTE
clearings, and having .huge_fault set makes vma_can_map_huge_file() report
THPeligible: 1 in /proc/<pid>/smaps for PMD-aligned COW mappings.  Should COW
mappings keep using a vm_ops structure that only defines
.access = generic_access_phys?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111113045.35368-1-farbere@amazon.com?part=4