Re: [PATCH 1/2] mm/userfaultfd: Fix uninitialized output field for -EAGAIN race

[David Hildenbrand <david@redhat.com>]

On 24.04.25 23:57, Peter Xu wrote:
> While discussing some userfaultfd relevant issues recently, Andrea noticed
> a potential ABI breakage with -EAGAIN on almost all userfaultfd ioctl()s.

I guess we talk about e.g., "man UFFDIO_COPY" documentation:

"The copy field is used by the kernel to return the number of bytes that 
was actually copied,  or an  error  (a  negated errno-style value).  The 
copy field is output-only; it is not read by the UFFDIO_COPY operation."

I assume -EINVAL/-ESRCH/-EFAULT are excluded from that rule, because 
there is no sense in user-space trying again on these errors either way. 
Well, there are cases where we would store -EFAULT, when we receive it 
from mfill_atomic_copy().

So if we store -EAGAIN to copy.copy it says "we didn't copy anything". 
(probably just storing 0 would have been better, but I am sure there was 
a reason to indicate negative errors in addition to returning an error)

> 
> Quote from Andrea, explaining how -EAGAIN was processed, and how this
> should fix it (taking example of UFFDIO_COPY ioctl):
> 
>    The "mmap_changing" and "stale pmd" conditions are already reported as
>    -EAGAIN written in the copy field, this does not change it. This change
>    removes the subnormal case that left copy.copy uninitialized and required
>    apps to explicitly set the copy field to get deterministic
>    behavior (which is a requirement contrary to the documentation in both
>    the manpage and source code). In turn there's no alteration to backwards
>    compatibility as result of this change because userland will find the
>    copy field consistently set to -EAGAIN, and not anymore sometime -EAGAIN
>    and sometime uninitialized.
> 
>    Even then the change only can make a difference to non cooperative users
>    of userfaultfd, so when UFFD_FEATURE_EVENT_* is enabled, which is not
>    true for the vast majority of apps using userfaultfd or this unintended
>    uninitialized field may have been noticed sooner.
> 
> Meanwhile, since this bug existed for years, it also almost affects all
> ioctl()s that was introduced later.  Besides UFFDIO_ZEROPAGE, these also
> get affected in the same way:
> 
>    - UFFDIO_CONTINUE
>    - UFFDIO_POISON
>    - UFFDIO_MOVE
> 
> This patch should have fixed all of them.
> 
> Fixes: df2cc96e7701 ("userfaultfd: prevent non-cooperative events vs mcopy_atomic races")
> Fixes: f619147104c8 ("userfaultfd: add UFFDIO_CONTINUE ioctl")
> Fixes: fc71884a5f59 ("mm: userfaultfd: add new UFFDIO_POISON ioctl")
> Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> Cc: linux-stable <stable@vger.kernel.org>
> Cc: Mike Rapoport <rppt@kernel.org>
> Cc: Axel Rasmussen <axelrasmussen@google.com>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Reported-by: Andrea Arcangeli <aarcange@redhat.com>
> Suggested-by: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   fs/userfaultfd.c | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index d80f94346199..22f4bf956ba1 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1585,8 +1585,11 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
>   	user_uffdio_copy = (struct uffdio_copy __user *) arg;
>   
>   	ret = -EAGAIN;
> -	if (atomic_read(&ctx->mmap_changing))
> +	if (unlikely(atomic_read(&ctx->mmap_changing))) {
> +		if (unlikely(put_user(ret, &user_uffdio_copy->copy)))
> +			return -EFAULT;
>   		goto out;
> +	}

Nit: It's weird that we do "return -EFAULT" in one case, in the other we 
do "goto out;" which ends up doing a "return ret" ...

Maybe to keep it consistent:

ret = -EAGAIN;
if (unlikely(atomic_read(&ctx->mmap_changing))) {
	if (unlikely(put_user(ret, &user_uffdio_copy->copy)))
		ret = -EFAULT;
    	goto out;
}


In all of these functions, we should probably just get rid of the "goto 
out" and just return directly. We have a weird mixture of "goto out;" 
and return; ... a different cleanup.


Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb