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?