[PATCH] mm/selftests: improve UFFD-WP feature detection in KSM test

[PATCH] mm/selftests: improve UFFD-WP feature detection in KSM test

[Li Wang]

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 error handling by:

1. Changes the feature check to first query supported features (features=0)
   rather than specifically requesting WP support.

2. Gracefully skipping the test if:
   - UFFDIO_API fails with EINVAL (feature not supported), or
   - UFFD_FEATURE_PAGEFAULT_FLAG_WP is not advertised by the kernel.

3. Providing better diagnostics by distinguishing expected failures (e.g.,
   EINVAL) from unexpected ones and reporting them using strerror().

The updated logic makes the test more robust across different kernel versions
and configurations, while preserving existing behavior on systems that do
support UFFD-WP.

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: David Hildenbrand <david@redhat.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>
---
 tools/testing/selftests/mm/ksm_functional_tests.c | 8 ++++++--
 1 file changed, 6 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..f3db257dc555 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("UFFDIO_API not supported (EINVAL)\n");
+		else
+			ksft_test_result_fail("UFFDIO_API failed: %s\n", strerror(errno));
+
 		goto close_uffd;
 	}
 	if (!(uffdio_api.features & UFFD_FEATURE_PAGEFAULT_FLAG_WP)) {
-- 
2.49.0

Re: [PATCH] mm/selftests: improve UFFD-WP feature detection in KSM test

[David Hildenbrand]

On 22.06.25 10:10, 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 error handling by:
> 
> 1. Changes the feature check to first query supported features (features=0)
>     rather than specifically requesting WP support.
> 
> 2. Gracefully skipping the test if:
>     - UFFDIO_API fails with EINVAL (feature not supported), or
>     - UFFD_FEATURE_PAGEFAULT_FLAG_WP is not advertised by the kernel.
> 
> 3. Providing better diagnostics by distinguishing expected failures (e.g.,
>     EINVAL) from unexpected ones and reporting them using strerror().
> 
> The updated logic makes the test more robust across different kernel versions
> and configurations, while preserving existing behavior on systems that do
> support UFFD-WP.
> 
> 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: David Hildenbrand <david@redhat.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>
> ---
>   tools/testing/selftests/mm/ksm_functional_tests.c | 8 ++++++--
>   1 file changed, 6 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..f3db257dc555 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("UFFDIO_API not supported (EINVAL)\n");
> +		else
> +			ksft_test_result_fail("UFFDIO_API failed: %s\n", strerror(errno));
> +
>   		goto close_uffd;
>   	}
>   	if (!(uffdio_api.features & UFFD_FEATURE_PAGEFAULT_FLAG_WP)) {


The man page (man UFFDIO_API) documents:

        Since  Linux  4.11,  applications should use the features field to perform a two-step handshake.
        First, UFFDIO_API is called with the features field set to zero.  The kernel responds by setting
        all supported feature bits.

        Applications which do not require any specific features can begin using the userfaultfd  immedi‐
        ately.   Applications which do need specific features should call UFFDIO_API again with a subset
        of the reported feature bits set to enable those features.

So likely, what you want in this patch here is something like:

diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
index b61803e36d1cf..5cf819ac958d0 100644
--- a/tools/testing/selftests/mm/ksm_functional_tests.c
+++ b/tools/testing/selftests/mm/ksm_functional_tests.c
@@ -393,7 +393,7 @@ 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");
                 goto close_uffd;
@@ -403,6 +403,14 @@ static void test_unmerge_uffd_wp(void)
                 goto close_uffd;
         }
  
+       /* Now, enable it ("two-step handshake") */
+       uffdio_api.api = UFFD_API;
+       uffdio_api.features = UFFD_FEATURE_PAGEFAULT_FLAG_WP;
+       if (ioctl(uffd, UFFDIO_API, &uffdio_api) < 0) {
+               ksft_test_result_fail("UFFDIO_API failed\n");
+               goto close_uffd;
+       }
+
         /* Register UFFD-WP, no need for an actual handler. */
         if (uffd_register(uffd, map, size, false, true, false)) {
                 ksft_test_result_fail("UFFDIO_REGISTER_MODE_WP failed\n");


-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/selftests: improve UFFD-WP feature detection in KSM test

[Li Wang]

[-- Attachment #1: Type: text/plain, Size: 6158 bytes --]

Hi David,

On Mon, Jun 23, 2025 at 4:33 PM David Hildenbrand <david@redhat.com> wrote:

> On 22.06.25 10:10, 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 error handling by:
> >
> > 1. Changes the feature check to first query supported features
> (features=0)
> >     rather than specifically requesting WP support.
> >
> > 2. Gracefully skipping the test if:
> >     - UFFDIO_API fails with EINVAL (feature not supported), or
> >     - UFFD_FEATURE_PAGEFAULT_FLAG_WP is not advertised by the kernel.
> >
> > 3. Providing better diagnostics by distinguishing expected failures
> (e.g.,
> >     EINVAL) from unexpected ones and reporting them using strerror().
> >
> > The updated logic makes the test more robust across different kernel
> versions
> > and configurations, while preserving existing behavior on systems that do
> > support UFFD-WP.
> >
> > 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: David Hildenbrand <david@redhat.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>
> > ---
> >   tools/testing/selftests/mm/ksm_functional_tests.c | 8 ++++++--
> >   1 file changed, 6 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..f3db257dc555 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("UFFDIO_API not supported
> (EINVAL)\n");
> > +             else
> > +                     ksft_test_result_fail("UFFDIO_API failed: %s\n",
> strerror(errno));
> > +
> >               goto close_uffd;
> >       }
> >       if (!(uffdio_api.features & UFFD_FEATURE_PAGEFAULT_FLAG_WP)) {
>
>
> The man page (man UFFDIO_API) documents:
>
>         Since  Linux  4.11,  applications should use the features field to
> perform a two-step handshake.
>         First, UFFDIO_API is called with the features field set to zero.
> The kernel responds by setting
>         all supported feature bits.
>
>         Applications which do not require any specific features can begin
> using the userfaultfd  immedi‐
>         ately.   Applications which do need specific features should call
> UFFDIO_API again with a subset
>         of the reported feature bits set to enable those features.
>

Thanks for pointing this out, very helpful.

Will send a patch v2 soon.



>
> So likely, what you want in this patch here is something like:
>
> diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c
> b/tools/testing/selftests/mm/ksm_functional_tests.c
> index b61803e36d1cf..5cf819ac958d0 100644
> --- a/tools/testing/selftests/mm/ksm_functional_tests.c
> +++ b/tools/testing/selftests/mm/ksm_functional_tests.c
> @@ -393,7 +393,7 @@ 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");
>                  goto close_uffd;
> @@ -403,6 +403,14 @@ static void test_unmerge_uffd_wp(void)
>                  goto close_uffd;
>          }
>
> +       /* Now, enable it ("two-step handshake") */
> +       uffdio_api.api = UFFD_API;
> +       uffdio_api.features = UFFD_FEATURE_PAGEFAULT_FLAG_WP;
> +       if (ioctl(uffd, UFFDIO_API, &uffdio_api) < 0) {
> +               ksft_test_result_fail("UFFDIO_API failed\n");
> +               goto close_uffd;
> +       }
> +
>          /* Register UFFD-WP, no need for an actual handler. */
>          if (uffd_register(uffd, map, size, false, true, false)) {
>                  ksft_test_result_fail("UFFDIO_REGISTER_MODE_WP failed\n");
>
>
> --
> Cheers,
>
> David / dhildenb
>
>

-- 
Regards,
Li Wang

[-- Attachment #2: Type: text/html, Size: 8645 bytes --]

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

[Li Wang]

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));
+
 		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);
+	uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
+	if (uffd < 0) {
+		ksft_test_result_skip("__NR_userfaultfd failed\n");
+		goto unmap;
+	}
+
+	/* Now, enable it ("two-step handshake") */
+	uffdio_api.api = UFFD_API;
+	uffdio_api.features = UFFD_FEATURE_PAGEFAULT_FLAG_WP;
+	if (ioctl(uffd, UFFDIO_API, &uffdio_api) < 0) {
+		ksft_test_result_fail("UFFDIO_API failed: %s\n", strerror(errno));
+		goto close_uffd;
+	}
+
 	/* Register UFFD-WP, no need for an actual handler. */
 	if (uffd_register(uffd, map, size, false, true, false)) {
 		ksft_test_result_fail("UFFDIO_REGISTER_MODE_WP failed\n");
-- 
2.49.0

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

[David Hildenbrand]

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.


-- 
Cheers,

David / dhildenb

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

[David Hildenbrand]

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

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

[Nadav Amit]

> 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.

> 
> 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;

I am not sure it is right since you would return EINVAL in this case.
It also looks a bit overly complicated - are you concerned about a race?
My whole concern about race was that somebody would exploit it to
overcome non-cooperative UFFD (IIRC).

So perhaps just add a check for the case features if 0 and be done with
it? Something like adding:

	ret = 0;
	if (ctx->features == 0 && features == 0)
		goto err_out; 		/* no error but copying of uffdio_api required */

before enabling the requested features for this uffd context.

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

[David Hildenbrand]

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)

> 
>>
>> 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;
> 
> I am not sure it is right since you would return EINVAL in this case.
> It also looks a bit overly complicated - are you concerned about a race?

Yes.

> My whole concern about race was that somebody would exploit it to
> overcome non-cooperative UFFD (IIRC).
> 
> So perhaps just add a check for the case features if 0 and be done with
> it? Something like adding:
> 
> 	ret = 0;
> 	if (ctx->features == 0 && features == 0)
> 		goto err_out; 		/* no error but copying of uffdio_api required */

Probably would also work. But let's find out first if we even want to 
fix this, given that it never seemed to have behaved that way from a 
quick glimpse.

-- 
Cheers,

David / dhildenb

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

[David Hildenbrand]

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

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

[Peter Xu]

On Tue, Jun 24, 2025 at 01:48:50PM +0200, David Hildenbrand wrote:
> 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?

The man-pages was inaccurate before. It got updated recently after Kyle
asking similar questions, see:

http://www.alejandro-colomar.es/src/alx/linux/man-pages/man-pages.git/commit/?id=6a949e9b08fd1ad4e77584276b9566e45fc07a93

Li's v2 change on using the temp fd looks correct.

Thanks,

-- 
Peter Xu

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

[David Hildenbrand]

On 24.06.25 17:03, Peter Xu wrote:
> On Tue, Jun 24, 2025 at 01:48:50PM +0200, David Hildenbrand wrote:
>> 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?
> 
> The man-pages was inaccurate before. It got updated recently after Kyle
> asking similar questions, see:

Ah, great, thanks!

-- 
Cheers,

David / dhildenb

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

[David Hildenbrand]

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));
> +
>   		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);
> +	uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
> +	if (uffd < 0) {
> +		ksft_test_result_skip("__NR_userfaultfd failed\n");

If it now suddenly fails (after it working above), this sure is a fail, 
right?

Apart from that

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

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

[Li Wang]

[-- Attachment #1: Type: text/plain, Size: 5334 bytes --]

On Tue, Jun 24, 2025 at 11:18 PM David Hildenbrand <david@redhat.com> 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));
> > +
> >               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);
> > +     uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
> > +     if (uffd < 0) {
> > +             ksft_test_result_skip("__NR_userfaultfd failed\n");
>
> If it now suddenly fails (after it working above), this sure is a fail,
> right?
>

Yes, you're right, here should ksft_test_result_fail().



>
> Apart from that
>
> Acked-by: David Hildenbrand <david@redhat.com>
>

Thanks for the review!


-- 
Regards,
Li Wang

[-- Attachment #2: Type: text/html, Size: 7784 bytes --]