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 <[email protected]> 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 <[email protected]>
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 <[email protected]>
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
