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

[David Hildenbrand <david@redhat.com>]

On 24.06.25 10:07, David Hildenbrand wrote:
> On 24.06.25 06:24, Li Wang wrote:
>> The current implementation of test_unmerge_uffd_wp() explicitly sets
>> `uffdio_api.features = UFFD_FEATURE_PAGEFAULT_FLAG_WP` before calling
>> UFFDIO_API. This can cause the ioctl() call to fail with EINVAL on kernels
>> that do not support UFFD-WP, leading the test to fail unnecessarily:
>>
>>     # ------------------------------
>>     # running ./ksm_functional_tests
>>     # ------------------------------
>>     # TAP version 13
>>     # 1..9
>>     # # [RUN] test_unmerge
>>     # ok 1 Pages were unmerged
>>     # # [RUN] test_unmerge_zero_pages
>>     # ok 2 KSM zero pages were unmerged
>>     # # [RUN] test_unmerge_discarded
>>     # ok 3 Pages were unmerged
>>     # # [RUN] test_unmerge_uffd_wp
>>     # not ok 4 UFFDIO_API failed     <-----
>>     # # [RUN] test_prot_none
>>     # ok 5 Pages were unmerged
>>     # # [RUN] test_prctl
>>     # ok 6 Setting/clearing PR_SET_MEMORY_MERGE works
>>     # # [RUN] test_prctl_fork
>>     # # No pages got merged
>>     # # [RUN] test_prctl_fork_exec
>>     # ok 7 PR_SET_MEMORY_MERGE value is inherited
>>     # # [RUN] test_prctl_unmerge
>>     # ok 8 Pages were unmerged
>>     # Bail out! 1 out of 8 tests failed
>>     # # Planned tests != run tests (9 != 8)
>>     # # Totals: pass:7 fail:1 xfail:0 xpass:0 skip:0 error:0
>>     # [FAIL]
>>
>> This patch improves compatibility and robustness of the UFFD-WP test
>> (test_unmerge_uffd_wp) by correctly implementing the UFFDIO_API
>> two-step handshake as recommended by the userfaultfd(2) man page.
>>
>> Key changes:
>>
>> 1. Use features=0 in the initial UFFDIO_API call to query supported
>>      feature bits, rather than immediately requesting WP support.
>>
>> 2. Skip the test gracefully if:
>>      - UFFDIO_API fails with EINVAL (e.g. unsupported API version), or
>>      - UFFD_FEATURE_PAGEFAULT_FLAG_WP is not advertised by the kernel.
>>
>> 3. Close the initial userfaultfd and create a new one before enabling
>>      the required feature, since UFFDIO_API can only be called once per fd.
>>
>> 4. Improve diagnostics by distinguishing between expected and unexpected
>>      failures, using strerror() to report errors.
>>
>> This ensures the test behaves correctly across a wider range of kernel
>> versions and configurations, while preserving the intended behavior on
>> kernels that support UFFD-WP.
>>
>> Suggestted-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Li Wang <liwang@redhat.com>
>> Cc: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
>> Cc: Bagas Sanjaya <bagasdotme@gmail.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>> Cc: Joey Gouly <joey.gouly@arm.com>
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: Keith Lucas <keith.lucas@oracle.com>
>> Cc: Ryan Roberts <ryan.roberts@arm.com>
>> Cc: Shuah Khan <shuah@kernel.org>
>> ---
>>
>> Notes:
>>       v1 --> v2:
>>       	* Close the original userfaultfd and open a new one before enabling features
>>       	* Reworked UFFDIO_API negotiation to follow the official two-step handshake
>>
>>    .../selftests/mm/ksm_functional_tests.c       | 28 +++++++++++++++++--
>>    1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
>> index b61803e36d1c..19e5b741893a 100644
>> --- a/tools/testing/selftests/mm/ksm_functional_tests.c
>> +++ b/tools/testing/selftests/mm/ksm_functional_tests.c
>> @@ -393,9 +393,13 @@ static void test_unmerge_uffd_wp(void)
>>    
>>    	/* See if UFFD-WP is around. */
>>    	uffdio_api.api = UFFD_API;
>> -	uffdio_api.features = UFFD_FEATURE_PAGEFAULT_FLAG_WP;
>> +	uffdio_api.features = 0;
>>    	if (ioctl(uffd, UFFDIO_API, &uffdio_api) < 0) {
>> -		ksft_test_result_fail("UFFDIO_API failed\n");
>> +		if (errno == EINVAL)
>> +			ksft_test_result_skip("The API version requested is not supported\n");
>> +		else
>> +			ksft_test_result_fail("UFFDIO_API failed: %s\n", strerror(errno));
>> +
> 
> Not sure if that is really required. If UFFDIO_API failed after
> __NR_userfaultfd worked something unexpected is happening.
> 
>>    		goto close_uffd;
>>    	}
>>    	if (!(uffdio_api.features & UFFD_FEATURE_PAGEFAULT_FLAG_WP)) {
>> @@ -403,6 +407,26 @@ static void test_unmerge_uffd_wp(void)
>>    		goto close_uffd;
>>    	}
>>    
>> +	/*
>> +	 * UFFDIO_API must only be called once to enable features.
>> +	 * So we close the old userfaultfd and create a new one to
>> +	 * actually enable UFFD_FEATURE_PAGEFAULT_FLAG_WP.
>> +	 */
>> +	close(uffd);
> 
> 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?

Without testing, the following might fix it if I am right:

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 22f4bf956ba1c..f03e7c980e1c5 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1944,9 +1944,9 @@ static int userfaultfd_move(struct userfaultfd_ctx *ctx,
  static int userfaultfd_api(struct userfaultfd_ctx *ctx,
                            unsigned long arg)
  {
+       unsigned int new_features, old_features = 0;
         struct uffdio_api uffdio_api;
         void __user *buf = (void __user *)arg;
-       unsigned int ctx_features;
         int ret;
         __u64 features;
  
@@ -1990,9 +1990,12 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
                 goto out;
  
         /* only enable the requested features for this uffd context */
-       ctx_features = uffd_ctx_features(features);
+       new_features = uffd_ctx_features(features);
+       /* allow two-step handshake */
+       if (userfaultfd_is_initialized(ctx))
+               old_features = UFFD_FEATURE_INITIALIZED;
         ret = -EINVAL;
-       if (cmpxchg(&ctx->features, 0, ctx_features) != 0)
+       if (cmpxchg(&ctx->features, old_features, new_features) != old_features)
                 goto err_out;
  
         ret = 0;


-- 
Cheers,

David / dhildenb