Re: [PATCH v8 13/15] userfaultfd: add UFFDIO_SET_MODE for runtime sync/async toggle

[sashiko-bot@kernel.org]

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

New issues:
- [High] The userfaultfd_set_mode ioctl iteratively traverses the VMA tree of a potentially external mm_struct without explicitly validating address space stability.
- [Medium] The UFFDIO_SET_MODE ioctl mutates ctx->features concurrently with userfaultfd_register() reading it locklessly.

Pre-existing issues:
- [High] Page tables are accessed and modified in userfaultfd_clear_vma before draining per-VMA lock readers.
--

commit f2f2d67be0ee3504658c8376634c7b20b76102e1
Author: Kiryl Shutsemau (Meta) <kas@kernel.org>

userfaultfd: add UFFDIO_SET_MODE for runtime sync/async toggle
    
This patch adds a new ioctl, UFFDIO_SET_MODE, to toggle userfaultfd features
at runtime, specifically the asynchronous mode for read-write protection (RWP).
It implements the logic to update context features under mmap_write_lock and
wakes pending faults so they retry under the new mode.

> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 1a3504c635972..a7659b0a1147e 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c

[ ... ]

> @@ -4297,6 +4307,109 @@ static int userfaultfd_rwprotect(struct userfaultfd_ctx *ctx,
>  	return ret;
>  }
>  
> +/* Subset of UFFD_API_FEATURES actually supported by this kernel/arch */
> +static __u64 uffd_api_available_features(void)

[ ... ]

> +static int userfaultfd_set_mode(struct userfaultfd_ctx *ctx,
> +				unsigned long arg)
> +{

[ ... ]

> +	/*
> +	 * Drain in-flight faults before flipping features. mmap_write_lock()
> +	 * blocks new mmap_read_lock() callers, but per-VMA locked faults
> +	 * (lock_vma_under_rcu() + FAULT_FLAG_VMA_LOCK) that acquired before
> +	 * this point keep running. Calling vma_start_write() on each UFFD-
> +	 * armed VMA waits for those readers to drop, so no in-flight fault
> +	 * can observe the old features after mmap_write_unlock().
> +	 */
> +	mmap_write_lock(mm);
> +	{
> +		struct vm_area_struct *vma;
> +		VMA_ITERATOR(vmi, mm, 0);
> +
> +		for_each_vma(vmi, vma) {

[Severity: High]
Does this code need to call check_stable_address_space(mm) before traversing
the VMA tree? Iterating the VMA tree of a potentially external mm_struct
without validating address space stability could crash the kernel if the mm
is undergoing teardown (like OOM or failed fork) and contains XA_ZERO_ENTRY
markers.

> +			if (vma->vm_userfaultfd_ctx.ctx == ctx)
> +				vma_start_write(vma);
> +		}
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the use of
vma_start_write() here, I noticed a pre-existing issue in
userfaultfd_clear_vma():

mm/userfaultfd.c:userfaultfd_clear_vma() {
...
	/* Clear the uffd bit and/or restore protnone PTEs */
	if (userfaultfd_protected(vma)) {
		unsigned int mm_cp_flags = 0;
		struct mmu_gather tlb;
...
		tlb_gather_mmu(&tlb, vma->vm_mm);
		change_protection(&tlb, vma, start, end, mm_cp_flags);
		tlb_finish_mmu(&tlb);
	}
...
}

Can accessing and modifying page tables via change_protection() before
calling vma_start_write() (which drains per-VMA lock readers) lead to
data corruption or use-after-free? A concurrent page fault or MADV_DONTNEED
under the per-VMA lock might observe intermediate page table states or
cleared entries.

> +	/*
> +	 * Single WRITE_ONCE so lockless readers (fdinfo, poll/read_iter
> +	 * via userfaultfd_is_initialized(), and the userfaultfd_features()
> +	 * helper used elsewhere) can't observe a mid-RMW intermediate
> +	 * value. Hot-path readers already serialise through the mmap lock
> +	 * + vma_start_write() drain above, so their load doesn't need an
> +	 * annotation.
> +	 */
> +	WRITE_ONCE(ctx->features,
> +		   (ctx->features | mode.enable) & ~mode.disable);

[Severity: Medium]
Could this concurrent write cause a data race with userfaultfd_register()?

userfaultfd_register() currently reads ctx->features natively:

mm/userfaultfd.c:userfaultfd_register() {
...
	if (uffdio_register.mode & UFFDIO_REGISTER_MODE_RWP) {
		if (!pgtable_supports_uffd() || VM_UFFD_RWP == VM_NONE)
			goto out;
		if (!(ctx->features & UFFD_FEATURE_RWP))
			goto out;
...
}

Should userfaultfd_register() be updated to use the new userfaultfd_features(ctx)
helper that safely uses READ_ONCE()?

> +	mmap_write_unlock(mm);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703133615.1039465-1-kirill@shutemov.name?part=13