Re: [PATCH v2] selftests/mm: Fix UFFDIO_API usage with proper two-step feature negotiation

[David Hildenbrand <david@redhat.com>]

On 24.06.25 13:39, David Hildenbrand wrote:
> On 24.06.25 13:29, Nadav Amit wrote:
>>
>>
>>> On 24 Jun 2025, at 11:22, David Hildenbrand <david@redhat.com> wrote:
>>>
>>> On 24.06.25 10:07, David Hildenbrand wrote:
>>>>>
>>>> Is that actually required?
>>>> The man page explicitly documents:
>>>> "       EINVAL A  previous  UFFDIO_API  call already enabled one or more
>>>> features for this userfaultfd.  Calling UFF‐
>>>>                  DIO_API twice, the first time with no features set, is
>>>> explicitly allowed as per the two-step  feature
>>>>                  detection handshake.
>>>> "
>>>> So if that doesn't work, something might be broken.
>>>
>>> CCing Nadav and Peter:
>>>
>>> Could it be that
>>>
>>> commit 22e5fe2a2a279d9a6fcbdfb4dffe73821bef1c90
>>> Author: Nadav Amit <nadav.amit@gmail.com>
>>> Date:   Thu Sep 2 14:58:59 2021 -0700
>>>
>>>      userfaultfd: prevent concurrent API initialization
>>>          userfaultfd assumes that the enabled features are set once and never
>>>      changed after UFFDIO_API ioctl succeeded.
>>>          However, currently, UFFDIO_API can be called concurrently from two
>>>      different threads, succeed on both threads and leave userfaultfd's
>>>      features in non-deterministic state.  Theoretically, other uffd operations
>>>      (ioctl's and page-faults) can be dispatched while adversely affected by
>>>      such changes of features.
>>>          Moreover, the writes to ctx->state and ctx->features are not ordered,
>>>      which can - theoretically, again - let userfaultfd_ioctl() think that
>>>      userfaultfd API completed, while the features are still not initialized.
>>>          To avoid races, it is arguably best to get rid of ctx->state.  Since there
>>>      are only 2 states, record the API initialization in ctx->features as the
>>>      uppermost bit and remove ctx->state.
>>>
>>> Accidentally broke the documented two-step handshake in the man page where we
>>> can avoid closing + reopening the fd?
>>
>> I agree the code is not correct (and my patch didn’t address this issue),
>> but I don’t see it broke it either.
>>
>> Unless I’m missing something the code before my patch, when
>> uffdio_api.features == 0, also set ctx->state to UFFD_STATE_RUNNING, which
>> meant another invocation would see (ctx->state != UFFD_STATE_WAIT_API) and
>> fail.
> 
> You might be right, I only checked the cmpxchg, assuming it was working
> before that.
> 
> ... but staring at the history of the "ctx->state =
> UFFD_STATE_RUNNING;", I am not sure if it ever behaved that way.
> 
> Do maybe, the man page is simply wrong (although I wonder why that case
> was described that detailed)

The man page was updated with

commit db3d5cc1a17b0ace008ebe1eaf0ac4d96b4b519a
Author: Axel Rasmussen <axelrasmussen@google.com>
Date:   Tue Oct 3 12:45:44 2023 -0700

     ioctl_userfaultfd.2: Correct and update UFFDIO_API ioctl error codes
     
     First, it is not correct that repeated UFFDIO_API calls result in
     EINVAL.  This is true *if both calls enable features*, but in the case
     where we're doing a two-step feature detection handshake, the kernel
     explicitly expects 2 calls (one with no features set).  So, correct this
     description.
     
     Then, some new error cases have been added to the kernel recently, and
     the man page wasn't updated to note these.  So, add in descriptions of
     these new error cases.

@Axel, did you ignore the automatically-set UFFD_FEATURE_INITIALIZED and the
repeated calls never worked, or was there actually a time where repeated
UFFDIO_API calls would not result in EINVAL?

-- 
Cheers,

David / dhildenb