[01/29] Revert "userfaultfd: don't fail on unrecognized features"

  This is a proposal to revert commit 914eedcb9ba0ff53c33808.

I found this when writting a simple UFFDIO_API test to be the first 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 != 0xaa.  This
  check is needed if the api version will be extended in the future, or
  user app won't be able to identify which is a new kernel.

  - Feature flags checks were removed, which means UFFDIO_API with a
  feature that does not exist will also succeed.  According to the man
  page, we should (and it makes sense) to reject ioctl(UFFDIO_API) if
  unknown features passed in.

Link: https://lore.kernel.org/r/20220722201513.1624158-1-axelrasmussen@google.com
Cc: Axel Rasmussen <axelrasmussen@google.com>
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 fs/userfaultfd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
  

On 30.03.23 17:56, Peter Xu wrote:
> This is a proposal to revert commit 914eedcb9ba0ff53c33808.
> 
> I found this when writting a simple UFFDIO_API test to be the first 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 != 0xaa.  This
>    check is needed if the api version will be extended in the future, or
>    user app won't be able to identify which is a new kernel.

Agreed.

> 
>    - Feature flags checks were removed, which means UFFDIO_API with a
>    feature that does not exist will also succeed.  According to the man
>    page, we should (and it makes sense) to reject ioctl(UFFDIO_API) if
>    unknown features passed in.
> 

Agreed.

I understand the motivation of the original commit, but it should not 
have changed existing checks/functionality. Introducing a different way 
to enable such functionality on explicit request would be better. But 
maybe simple feature probing (is X support? is Y supported? is Z 
supported) might be easier without requiring ABI changes.

I assume we better add

Fixes: 914eedcb9ba0 ("userfaultfd: don't fail on unrecognized features")


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

> Link: https://lore.kernel.org/r/20220722201513.1624158-1-axelrasmussen@google.com
> Cc: Axel Rasmussen <axelrasmussen@google.com>
> Cc: linux-stable <stable@vger.kernel.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   fs/userfaultfd.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 8395605790f6..3b2a41c330e6 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1977,8 +1977,10 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
>   	ret = -EFAULT;
>   	if (copy_from_user(&uffdio_api, buf, sizeof(uffdio_api)))
>   		goto out;
> -	/* Ignore unsupported features (userspace built against newer kernel) */
> -	features = uffdio_api.features & UFFD_API_FEATURES;
> +	features = uffdio_api.features;
> +	ret = -EINVAL;
> +	if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES))
> +		goto err_out;
>   	ret = -EPERM;
>   	if ((features & UFFD_FEATURE_EVENT_FORK) && !capable(CAP_SYS_PTRACE))
>   		goto err_out;
  

On Thu, Mar 30, 2023 at 8:57 AM 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 first 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 != 0xaa.  This
>   check is needed if the api version will be extended in the future, or
>   user app won't be able to identify which is a new kernel.

100% agreed, this was a mistake.

>
>   - Feature flags checks were removed, which means UFFDIO_API with a
>   feature that does not exist will also succeed.  According to the man
>   page, we should (and it makes sense) to reject ioctl(UFFDIO_API) if
>   unknown features passed in.

I still strongly disagree with reverting this part, my feeling is
still that doing so makes things more complicated for no reason.

Re: David's point, it's clearly wrong to change semantics so a thing
that used to work now fails. But this instead makes it more permissive
- existing userspace programs continue to work as-is, but *also* one
can achieve the same thing more simply (combine probing +
configuration into one step). I don't see any problem with that,
generally.

But, if David and others don't find my argument convincing, it isn't
the end of the world. It just means I have to go update my userspace
code to be a bit more complicated. :)

I also still think the man page is incorrect or at least incomplete no
matter what we do here; we should be sure to update it as a follow-up.

>
> Link: https://lore.kernel.org/r/20220722201513.1624158-1-axelrasmussen@google.com
> Cc: Axel Rasmussen <axelrasmussen@google.com>
> Cc: linux-stable <stable@vger.kernel.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  fs/userfaultfd.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 8395605790f6..3b2a41c330e6 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1977,8 +1977,10 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
>         ret = -EFAULT;
>         if (copy_from_user(&uffdio_api, buf, sizeof(uffdio_api)))
>                 goto out;
> -       /* Ignore unsupported features (userspace built against newer kernel) */
> -       features = uffdio_api.features & UFFD_API_FEATURES;
> +       features = uffdio_api.features;
> +       ret = -EINVAL;
> +       if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES))
> +               goto err_out;
>         ret = -EPERM;
>         if ((features & UFFD_FEATURE_EVENT_FORK) && !capable(CAP_SYS_PTRACE))
>                 goto err_out;
> --
> 2.39.1
>
  

On Thu, Mar 30, 2023 at 08:31:30PM +0200, David Hildenbrand wrote:
> On 30.03.23 17:56, Peter Xu wrote:
> > This is a proposal to revert commit 914eedcb9ba0ff53c33808.
> > 
> > I found this when writting a simple UFFDIO_API test to be the first 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 != 0xaa.  This
> >    check is needed if the api version will be extended in the future, or
> >    user app won't be able to identify which is a new kernel.
> 
> Agreed.
> 
> > 
> >    - Feature flags checks were removed, which means UFFDIO_API with a
> >    feature that does not exist will also succeed.  According to the man
> >    page, we should (and it makes sense) to reject ioctl(UFFDIO_API) if
> >    unknown features passed in.
> > 
> 
> Agreed.
> 
> I understand the motivation of the original commit, but it should not have
> changed existing checks/functionality. Introducing a different way to enable
> such functionality on explicit request would be better. But maybe simple
> feature probing (is X support? is Y supported? is Z supported) might be
> easier without requiring ABI changes.

Yes, I mentioned a similar "proposal" of UFFDIO_FEATURES here too, simply
returning the feature bitmask before UFFDIO_API:

https://lore.kernel.org/all/ZCSUTSbAcwBINiNk@x1n/

But I think current way is still fine; so maybe we'd just not bother.

> 
> I assume we better add
> 
> Fixes: 914eedcb9ba0 ("userfaultfd: don't fail on unrecognized features")

Yes I'll add it.

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

Thanks,
  

On Thu, Mar 30, 2023 at 12:04:09PM -0700, Axel Rasmussen wrote:
> On Thu, Mar 30, 2023 at 8:57 AM 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 first 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 != 0xaa.  This
> >   check is needed if the api version will be extended in the future, or
> >   user app won't be able to identify which is a new kernel.
> 
> 100% agreed, this was a mistake.
> 
> >
> >   - Feature flags checks were removed, which means UFFDIO_API with a
> >   feature that does not exist will also succeed.  According to the man
> >   page, we should (and it makes sense) to reject ioctl(UFFDIO_API) if
> >   unknown features passed in.
> 
> I still strongly disagree with reverting this part, my feeling is
> still that doing so makes things more complicated for no reason.
> 
> Re: David's point, it's clearly wrong to change semantics so a thing
> that used to work now fails. But this instead makes it more permissive
> - existing userspace programs continue to work as-is, but *also* one
> can achieve the same thing more simply (combine probing +
> configuration into one step). I don't see any problem with that,
> generally.
> 
> But, if David and others don't find my argument convincing, it isn't
> the end of the world. It just means I have to go update my userspace
> code to be a bit more complicated. :)

I'd say it's fine if it's your own program that you can in full control
easily. :) Sorry again for not noticing that earlier.

There's one reason that we may consider keeping the behavior.  IMHO it 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 patch,
please shoot.

> 
> I also still think the man page is incorrect or at least incomplete no
> matter what we do here; we should be sure to update it as a follow-up.

So far it looks still fine if with this revert.  Maybe I overlooked
somewhere?

I'll add this into my todo, but with low priority.  If you have suggestion
already on how to improve the man page please do so before me!
  

On Thu, Mar 30, 2023 at 3:27 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, Mar 30, 2023 at 12:04:09PM -0700, Axel Rasmussen wrote:
> > On Thu, Mar 30, 2023 at 8:57 AM 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 first 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 != 0xaa.  This
> > >   check is needed if the api version will be extended in the future, or
> > >   user app won't be able to identify which is a new kernel.
> >
> > 100% agreed, this was a mistake.
> >
> > >
> > >   - Feature flags checks were removed, which means UFFDIO_API with a
> > >   feature that does not exist will also succeed.  According to the man
> > >   page, we should (and it makes sense) to reject ioctl(UFFDIO_API) if
> > >   unknown features passed in.
> >
> > I still strongly disagree with reverting this part, my feeling is
> > still that doing so makes things more complicated for no reason.
> >
> > Re: David's point, it's clearly wrong to change semantics so a thing
> > that used to work now fails. But this instead makes it more permissive
> > - existing userspace programs continue to work as-is, but *also* one
> > can achieve the same thing more simply (combine probing +
> > configuration into one step). I don't see any problem with that,
> > generally.
> >
> > But, if David and others don't find my argument convincing, it isn't
> > the end of the world. It just means I have to go update my userspace
> > code to be a bit more complicated. :)
>
> I'd say it's fine if it's your own program that you can in full control
> easily. :) Sorry again for not noticing that earlier.
>
> There's one reason that we may consider keeping the behavior.  IMHO it 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 patch,
> please shoot.

Well, I did find one example, criu:
https://github.com/checkpoint-restore/criu/blob/criu-dev/criu/uffd.c#L266
It doesn't do the two-step probing process, it looks to me like it
does what my patch was intending users to do:

It just asks for the requested features up-front, without any probing.
And then it returns the subset of features it *actually* got,
ostensibly so the caller can compare that vs. what it requested.

Then again, it looks like the caller doesn't *actually* compare the
features it got vs. what it asked for. I don't know enough about criu
to know if this is a bug, or if they actually just don't care.
https://github.com/checkpoint-restore/criu/blob/criu-dev/criu/uffd.c#L312

>
> >
> > I also still think the man page is incorrect or at least incomplete no
> > matter what we do here; we should be sure to update it as a follow-up.
>
> So far it looks still fine if with this revert.  Maybe I overlooked
> somewhere?
>
> I'll add this into my todo, but with low priority.  If you have suggestion
> already on how to improve the man page please do so before me!

My thinking is that it describes the bits and pieces, but doesn't
explicitly describe end-to-end how to configure a userfaultfd using
the two-step probing approach. (Or state that this is actually
*required*, unless you only want to set features=0 in any case.)

Maybe it will be easiest if I just send a patch myself with what I'm
thinking, and we can see what folks think. Always easier to just look
at a patch vs. talking about it in the abstract. :)

>
> --
> Peter Xu
>
  

On Fri, 31 Mar 2023 at 17:52, Axel Rasmussen <axelrasmussen@google.com> wrote:
>
> On Thu, Mar 30, 2023 at 3:27 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Thu, Mar 30, 2023 at 12:04:09PM -0700, Axel Rasmussen wrote:
> > > On Thu, Mar 30, 2023 at 8:57 AM 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 first 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 != 0xaa.  This
> > > >   check is needed if the api version will be extended in the future, or
> > > >   user app won't be able to identify which is a new kernel.
> > > >
> > > >   - Feature flags checks were removed, which means UFFDIO_API with a
> > > >   feature that does not exist will also succeed.  According to the 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/

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=8917a777be3b
(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#designing-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

[..]
> > There's one reason that we may consider keeping the behavior.  IMHO it 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 patch,
> > please shoot.
>
> Well, I did find one example, criu:
> https://github.com/checkpoint-restore/criu/blob/criu-dev/criu/uffd.c#L266

Mike can speak better than me about uffd, but AFAICS, CRIU correctly detects
features with kerneldat/kdat:
https://github.com/checkpoint-restore/criu/blob/criu-dev/criu/kerndat.c#L1235

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

Thanks,
             Dmitry
  

On Fri, Mar 31, 2023 at 11:08 AM Dmitry Safonov <0x7f454c46@gmail.com> wrote:
>
> On Fri, 31 Mar 2023 at 17:52, Axel Rasmussen <axelrasmussen@google.com> wrote:
> >
> > On Thu, Mar 30, 2023 at 3:27 PM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Thu, Mar 30, 2023 at 12:04:09PM -0700, Axel Rasmussen wrote:
> > > > On Thu, Mar 30, 2023 at 8:57 AM 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 first 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 != 0xaa.  This
> > > > >   check is needed if the api version will be extended in the future, or
> > > > >   user app won't be able to identify which is a new kernel.
> > > > >
> > > > >   - Feature flags checks were removed, which means UFFDIO_API with a
> > > > >   feature that does not exist will also succeed.  According to the 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=8917a777be3b
> (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#designing-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 it 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 patch,
> > > please shoot.
> >
> > Well, I did find one example, criu:
> > https://github.com/checkpoint-restore/criu/blob/criu-dev/criu/uffd.c#L266
>
> Mike can speak better than me about uffd, but AFAICS, CRIU correctly detects
> features with kerneldat/kdat:
> https://github.com/checkpoint-restore/criu/blob/criu-dev/criu/kerndat.c#L1235

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
  

On 30.03.23 21:04, Axel Rasmussen wrote:
> On Thu, Mar 30, 2023 at 8:57 AM 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 first 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 != 0xaa.  This
>>    check is needed if the api version will be extended in the future, or
>>    user app won't be able to identify which is a new kernel.
> 
> 100% agreed, this was a mistake.
> 
>>
>>    - Feature flags checks were removed, which means UFFDIO_API with a
>>    feature that does not exist will also succeed.  According to the man
>>    page, we should (and it makes sense) to reject ioctl(UFFDIO_API) if
>>    unknown features passed in.
> 
> I still strongly disagree with reverting this part, my feeling is
> still that doing so makes things more complicated for no reason.
> 
> Re: David's point, it's clearly wrong to change semantics so a thing
> that used to work now fails. But this instead makes it more permissive
> - existing userspace programs continue to work as-is, but *also* one
> can achieve the same thing more simply (combine probing +
> configuration into one step). I don't see any problem with that,
> generally.
> 
> But, if David and others don't find my argument convincing, it isn't
> the end of the world. It just means I have to go update my userspace
> code to be a bit more complicated. :)


I'd probably find it more convincing if we'd started out with that 
approach ;) . User space would have to deal with the behavior of old 
kernels either way already? IOW, old kernels would reject the new flags, 
new kernels would not reject them but mask them out. So changing that 
behavior after the effects is somewhat suboptimal IMHO ... and rather 
makes things more complicated.