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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADrL8HVhMhG6_nSwLfVr4g8XpjA9xh+maLPrC1=jv+L6LNxxkA@mail.gmail.com>
Date: Fri, 25 Apr 2025 11:54:47 -0400
From: James Houghton <jthoughton@...gle.com>
To: David Hildenbrand <david@...hat.com>
Cc: Peter Xu <peterx@...hat.com>, linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	Mike Rapoport <rppt@...nel.org>, Suren Baghdasaryan <surenb@...gle.com>, 
	Axel Rasmussen <axelrasmussen@...gle.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	linux-stable <stable@...r.kernel.org>, Andrea Arcangeli <aarcange@...hat.com>
Subject: Re: [PATCH 1/2] mm/userfaultfd: Fix uninitialized output field for
 -EAGAIN race

On Fri, Apr 25, 2025 at 11:12 AM David Hildenbrand <david@...hat.com> wrote:
>
> 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)

IMHO, it makes more sense to store 0 than -EAGAIN (at least it will
mean that my userspace[1] won't break).

Userspace will need to know from where to restart the ioctl, and if we
put -EAGAIN in `mapped`/`copy`/etc., userspace will need to know that
-EAGAIN actually means 0 anyway.

[1]: https://lore.kernel.org/linux-mm/CADrL8HXuZkX0CP6apHLw0A0Ax4b4a+-=XEt0dH5mAKiN7hBv3w@mail.gmail.com/