Re: [PATCH] userfaultfd: don't fail on unrecognized features

[Peter Xu]

On Wed, Mar 29, 2023 at 10:53:38AM -0700, Axel Rasmussen wrote:
> On Tue, Mar 28, 2023 at 3:34 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
> >
> > On Tue, Mar 28, 2023 at 02:52:35PM -0700, Axel Rasmussen wrote:
> > > I don't see being very strict here as useful. Another example might be
> > > madvise() - for example trying to MADV_PAGEOUT on a kernel that
> > > doesn't support it. There is no way the kernel can proceed here, since
> > > it simply doesn't know how to do what you're asking for. In this case
> > > an error makes sense.
> >
> > IMHO, PAGEOUT is not a great example.  I wished we can have a way to probe
> > what madvise() the system supports, and I know many people wanted that too.
> > I even had a feeling that we'll have it some day.
> >
> > So now I'm going back to look at this patch assuming I'm reviewing it, I'm
> > still not convinced the old API needs changing.
> >
> > Userfaultfd allows probing with features=0 with/without this patch, so I
> > see this patch as something that doesn't bring a direct functional benefit,
> 
> The benefit is we combine probing for features and creating a
> userfaultfd into a single step, so userspace doesn't have to open +
> manipulate a userfaultfd twice. In my mind, both approaches achieve
> the same thing, it's just that one requires extra steps to get there.
> 
> To me, it's still unclear why there is any harm in supporting the
> simpler way? And, I also don't see any way in which the more complex
> way is better?

Because that's what the man page says? :)

> 
> > but some kind of api change due to subjective preferences which I cannot
> > say right or wrong.  Now the patch is already merged.  If we need to change
> > either this patch or the man page to make them match again, again I'd
> > prefer we simply revert it to keep everything like before and copy stable.
> 
> I think we need to change documentation either way. But, I think the
> changes needed are actually bigger if we want to revert.

IIUC the man page doesn't need to update if we revert this patch.

The man page described clearly on what will happen if we pass in feature
bits that are not supported:

       To enable userfaultfd features the application should set a bit
       corresponding to each feature it wants to enable in the features
       field.  If the kernel supports all the requested features it will
       enable them.  Otherwise it will zero out the returned uffdio_api
       structure and return EINVAL.

> With the simpler behavior, the selftest and the example program in the
> man page are ~correct as-is; otherwise we would need to modify those
> to use the two-step probing method.
> 
> (By the way, I am excited about the selftest refactoring you talked
> about! Thanks for doing that work. It definitely needs it, the
> complexity there has gotten significantly worse as we've added more
> things onto it [wp, minor faults].)

I'll definitely copy you when I post it.  It growed a bit larger than I
thought, it'll be great if you can help have a look.

In the test cases I added an UFFDIO_API test to be the 1st one and that's
why I found this issue.  To let all tests pass currently I'll need to
revert this patch.  If you want we can move the discussion there when I
post it, I think that may need to be the 1st patch for the test suite
change and to let current test suite pass.

> I think the man page description of how to use the API is incomplete
> in either case. Right now it sort of alludes to the fact that you can
> probe with features==0, but it doesn't explicitly say "you need to
> probe first, then close that userfaultfd and open the real one you
> want to use, with a subset of the features reported in the first
> step". If we want to keep the old behavior, it should be more explicit
> about the steps needed to get a userfaultfd.

To tell the truth, if I'm going to change the API anyway, I'll simply add a
UFFDIO_FEATURES ioctl() returning the supported features, that'll be much,
much easier than either the old one or the one this patch proposed, IMHO.
Then we keep all the rest untouched.  That should work perfectly and
that'll not require open()/close() duplications either.

But as I mentioned before, I don't think UFFDIO_FEATURES justifies itself
either much on being worthwhile because it introduces a new ioctl without
any major benefit.  At least we'll need to keep the old behavior still
working.

> 
> You are right that it also doesn't describe "you can just ask for what
> you want, and the kernel tells you what subset it can give you; you
> need to check that the reported features are acceptable" - the new
> behavior. That should be updated.

-- 
Peter Xu