Re: [PATCH v4 12/14] userfaultfd: add UFFDIO_SET_MODE for runtime sync/async toggle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The userfaultfd_set_mode function iterates over an external mm_struct's VMAs without verifying address space stability.
- [Low] Lockless read of ctx->features races with its own WRITE_ONCE update, causing a data race.
--

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

userfaultfd: add UFFDIO_SET_MODE for runtime sync/async toggle

This commit adds an ioctl to toggle UFFD_FEATURE_RWP_ASYNC on an active 
userfaultfd context. It handles synchronization by acquiring the mmap_write_lock,
draining in-flight faults via vma_start_write(), and waking pending sync
waiters to retry.

> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 20478bb373111..680ef9bd57fd6 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c

[ ... ]

> @@ -4261,6 +4271,109 @@ static int userfaultfd_rwprotect(struct userfaultfd_ctx *ctx,
>  	return ret;
>  }
>  

[ ... ]

> +static int userfaultfd_set_mode(struct userfaultfd_ctx *ctx,
> +				unsigned long arg)
> +{
> +	struct uffdio_set_mode mode;
> +	struct mm_struct *mm = ctx->mm;
> +
> +	if (copy_from_user(&mode, (void __user *)arg, sizeof(mode)))
> +		return -EFAULT;
> +
> +	/* enable and disable must not overlap */
> +	if (mode.enable & mode.disable)
> +		return -EINVAL;
> +
> +	/* only toggleable features that this kernel/arch actually supports */
> +	if ((mode.enable | mode.disable) &
> +	    ~(uffd_api_available_features() & UFFD_FEATURE_TOGGLEABLE))
> +		return -EINVAL;
> +
> +	/* RWP_ASYNC can only be enabled on contexts that negotiated RWP */
> +	if ((mode.enable & UFFD_FEATURE_RWP_ASYNC) &&
> +	    !(ctx->features & UFFD_FEATURE_RWP))

[Severity: Low]
Is it possible for a concurrent UFFDIO_SET_MODE call to trigger a data race
here?

Because ctx->features is updated with WRITE_ONCE() later in this function,
reading it locklessly without the new userfaultfd_features() helper
(or READ_ONCE) might cause KCSAN warnings, even though the UFFD_FEATURE_RWP
bit itself doesn't change.

> +		return -EINVAL;
> +
> +	if (!mmget_not_zero(mm))
> +		return -ESRCH;
> +
> +	/*
> +	 * 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 iteration code need to verify address space stability before
walking the VMAs?

Since ctx->mm can belong to a different process, it might be marked
MMF_UNSTABLE if the target process encountered a dup_mmap() failure or
was reaped by the OOM killer.

If the address space is unstable, the maple tree might contain XA_ZERO_ENTRY
markers, which could cause a kernel panic when dereferencing
vma->vm_userfaultfd_ctx.ctx.

Would it be appropriate to call check_stable_address_space(mm) right after
acquiring the mmap_write_lock?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525113737.1942478-1-kas@kernel.org?part=12