Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230330155707.3106228-1-peterx@redhat.com> <20230330155707.3106228-2-peterx@redhat.com>
 <CAJHvVcgDZBi6pH0BD12sQ3T+7Kr9exX1QU3-YLTd1voYhVBN0w@mail.gmail.com>
 <ZCYMu5P2BJy/2z5t@x1n> <CAJHvVcggL+s=WEGzwR8+QvWgZANiLut+DhmosKtAXZ1F2vtFAg@mail.gmail.com>
 <CAJwJo6YrfDH5-Tdsbau-AevVUuqiDQE74se3XvenT20Fbrrcnw@mail.gmail.com>
In-Reply-To: <CAJwJo6YrfDH5-Tdsbau-AevVUuqiDQE74se3XvenT20Fbrrcnw@mail.gmail.com>
From:   Axel Rasmussen <axelrasmussen@google.com>
Date:   Fri, 31 Mar 2023 13:04:53 -0700
Message-ID: <CAJHvVcgiLbACcCr1O4ng7vrxC1Sok_HXDuzbvnVyAaeqGfdwuw@mail.gmail.com>
Subject: Re: [PATCH 01/29] Revert "userfaultfd: don't fail on unrecognized features"
To:     Dmitry Safonov <0x7f454c46@gmail.com>
Cc:     Peter Xu <peterx@redhat.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Mike Rapoport <rppt@linux.vnet.ibm.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        Leonardo Bras Soares Passos <lsoaresp@redhat.com>,
        David Hildenbrand <david@redhat.com>,
        linux-stable <stable@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Mar 31, 2023 at 11:08=E2=80=AFAM Dmitry Safonov <0x7f454c46@gmail.c=
om> wrote:
>
> On Fri, 31 Mar 2023 at 17:52, Axel Rasmussen <axelrasmussen@google.com> w=
rote:
> >
> > On Thu, Mar 30, 2023 at 3:27=E2=80=AFPM Peter Xu <peterx@redhat.com> wr=
ote:
> > >
> > > On Thu, Mar 30, 2023 at 12:04:09PM -0700, Axel Rasmussen wrote:
> > > > On Thu, Mar 30, 2023 at 8:57=E2=80=AFAM Peter Xu <peterx@redhat.com=
> wrote:
> > > > >
> > > > > This is a proposal to revert commit 914eedcb9ba0ff53c33808.
> > > > >
> > > > > I found this when writting a simple UFFDIO_API test to be the fir=
st unit
> > > > > test in this set.  Two things breaks with the commit:
> > > > >
> > > > >   - UFFDIO_API check was lost and missing.  According to man page=
, the
> > > > >   kernel should reject ioctl(UFFDIO_API) if uffdio_api.api !=3D 0=
xaa.  This
> > > > >   check is needed if the api version will be extended in the futu=
re, or
> > > > >   user app won't be able to identify which is a new kernel.
> > > > >
> > > > >   - Feature flags checks were removed, which means UFFDIO_API wit=
h a
> > > > >   feature that does not exist will also succeed.  According to th=
e man
> > > > >   page, we should (and it makes sense) to reject ioctl(UFFDIO_API=
) if
> > > > >   unknown features passed in.
>
> If features/flags are not checked in kernel, and the kernel doesn't
> return an error on
> an unknown flag/error, that makes the syscall non-extendable, meaning
> that adding
> any new feature may break existing software, which doesn't sanitize
> them properly.
> https://lwn.net/Articles/588444/

I don't think the same problem applies here. In the case of syscalls,
the problem is the only way the kernel can communicate is by the
EINVAL return value. Without the check, if a call succeeds the caller
can't tell: was the flag supported + applied, or unrecognized +
ignored?

With UFFDIO_API (we aren't talking about userfaultfd(2) itself), when
you pass in a set of flags, we return the subset of flags which were
enabled, in addition to the return code. So via that mechanism, one is
"able to check whether it is running on a kernel where [userfaultfd]
supports [the feature]" as the article describes - the only difference
is, the caller must check the returned set of features, instead of
checking for an error code. I don't think it's exactly *how* userspace
can check that's important, but rather *that* it can check.

Another important difference: I have a hard time imagining a case
where adding a new feature could break userspace, even with my
approach, but let's say for the sake of argument one arises in the
future. Unlike normal syscalls, we have the UFFD_API version check, so
we have the option of incrementing that to separate users relying on
the old behavior, from users willing to deal with the new behavior.

(Syscalls can kind of replicate this by adding a new syscall, like
clone() vs clone2(), but I think that's messier than the API version
check being built-in to the API.)

>
> See a bunch of painful exercises from syscalls with numbers in the end:
> https://lwn.net/Articles/792628/
> To adding an additional setsockopt() because an old one didn't have
> sanity checks for flags:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit=
/?id=3D8917a777be3b
> (not the best example, as the new setsockopt() didn't check flags for
> sanity as well (sic!),
> but that's near the code I work on now)
>
> This is even documented nowadays:
> https://www.kernel.org/doc/html/latest/process/adding-syscalls.html#desig=
ning-the-api-planning-for-extension
>
> ...and everyone knows what happens when you blame userspace for breaking =
by
> not doing what you would have expected it to do:
> https://lkml.org/lkml/2012/12/23/75

100% agreed. :)

>
> [..]
> > > There's one reason that we may consider keeping the behavior.  IMHO i=
t is
> > > when there're major softwares that uses the "wrong" ABI (let's say so=
;
> > > because it's not following the man pages).  If you're aware any such =
major
> > > softwares (especially open sourced) will break due to this revert pat=
ch,
> > > please shoot.
> >
> > Well, I did find one example, criu:
> > https://github.com/checkpoint-restore/criu/blob/criu-dev/criu/uffd.c#L2=
66
>
> Mike can speak better than me about uffd, but AFAICS, CRIU correctly dete=
cts
> features with kerneldat/kdat:
> https://github.com/checkpoint-restore/criu/blob/criu-dev/criu/kerndat.c#L=
1235

Ah, right, this is the simplest case where no optional features are
asked for. So, it's not a great example; this particular case would
look the same regardless of what the kernel does.

>
> So, doing a sane thing in kernel shouldn't break CRIU (at least here).
>
> Thanks,
>              Dmitry