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] [day] [month] [year] [list]

Message-ID: <aAu31E7CSbhw6Yh9@x1.local>
Date: Fri, 25 Apr 2025 12:27:00 -0400
From: Peter Xu <peterx@...hat.com>
To: James Houghton <jthoughton@...gle.com>
Cc: David Hildenbrand <david@...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:54:47AM -0400, James Houghton wrote:
> 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.

Yes agreed, the API might be easier to follow if the kernel will only
update >=0 values to copy.copy and only if -EAGAIN is returned, so that
errno will be the only source of truth on the type of error that userapp
must check first. For now, we may need to stick with the current API.

-- 
Peter Xu