[PATCH v2 1/3] Fix userfaultfd_api to return EINVAL as expected

[PATCH v2 1/3] Fix userfaultfd_api to return EINVAL as expected

[Audra Mitchell]

Currently if we request a feature that is not set in the Kernel
config we fail silently and return all the available features. However,
the man page indicates we should return an EINVAL.

We need to fix this issue since we can end up with a Kernel warning
should a program request the feature UFFD_FEATURE_WP_UNPOPULATED on
a kernel with the config not set with this feature.

 [  200.812896] WARNING: CPU: 91 PID: 13634 at mm/memory.c:1660 zap_pte_range+0x43d/0x660
 [  200.820738] Modules linked in:
 [  200.869387] CPU: 91 PID: 13634 Comm: userfaultfd Kdump: loaded Not tainted 6.9.0-rc5+ #8
 [  200.877477] Hardware name: Dell Inc. PowerEdge R6525/0N7YGH, BIOS 2.7.3 03/30/2022
 [  200.885052] RIP: 0010:zap_pte_range+0x43d/0x660

Fixes: e06f1e1dd499 ("userfaultfd: wp: enabled write protection in userfaultfd API")
Signed-off-by: Audra Mitchell <audra@redhat.com>
---
 fs/userfaultfd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index eee7320ab0b0..17e409ceaa33 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -2057,7 +2057,7 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
 		goto out;
 	features = uffdio_api.features;
 	ret = -EINVAL;
-	if (uffdio_api.api != UFFD_API || (features & ~UFFD_API_FEATURES))
+	if (uffdio_api.api != UFFD_API)
 		goto err_out;
 	ret = -EPERM;
 	if ((features & UFFD_FEATURE_EVENT_FORK) && !capable(CAP_SYS_PTRACE))
@@ -2081,6 +2081,11 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx,
 	uffdio_api.features &= ~UFFD_FEATURE_WP_UNPOPULATED;
 	uffdio_api.features &= ~UFFD_FEATURE_WP_ASYNC;
 #endif
+
+	ret = -EINVAL;
+	if (features & ~uffdio_api.features)
+		goto err_out;
+
 	uffdio_api.ioctls = UFFD_API_IOCTLS;
 	ret = -EFAULT;
 	if (copy_to_user(buf, &uffdio_api, sizeof(uffdio_api)))
-- 
2.44.0

[PATCH v2 2/3] Update uffd-stress to handle EINVAL for unset config features

[Audra Mitchell]

Now that we have updated userfaultfd_api to correctly return
EIVAL when a feature is requested but not available, let's fix
the uffd-stress test to only set the UFFD_FEATURE_WP_UNPOPULATED
feature when the config is set. In addition, still run the test if
the CONFIG_PTE_MARKER_UFFD_WP is not set, just dont use the corresponding
UFFD_FEATURE_WP_UNPOPULATED feature.

Signed-off-by: Audra Mitchell <audra@redhat.com>
---
 tools/testing/selftests/mm/uffd-stress.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
index f78bab0f3d45..b9b6d858eab8 100644
--- a/tools/testing/selftests/mm/uffd-stress.c
+++ b/tools/testing/selftests/mm/uffd-stress.c
@@ -38,6 +38,8 @@
 
 #ifdef __NR_userfaultfd
 
+uint64_t features;
+
 #define BOUNCE_RANDOM		(1<<0)
 #define BOUNCE_RACINGFAULTS	(1<<1)
 #define BOUNCE_VERIFY		(1<<2)
@@ -247,10 +249,14 @@ static int userfaultfd_stress(void)
 	unsigned long nr;
 	struct uffd_args args[nr_cpus];
 	uint64_t mem_size = nr_pages * page_size;
+	int flags = 0;
 
 	memset(args, 0, sizeof(struct uffd_args) * nr_cpus);
 
-	if (uffd_test_ctx_init(UFFD_FEATURE_WP_UNPOPULATED, NULL))
+	if (features & UFFD_FEATURE_WP_UNPOPULATED && test_type == TEST_ANON)
+		flags = UFFD_FEATURE_WP_UNPOPULATED;
+
+	if (uffd_test_ctx_init(flags, NULL))
 		err("context init failed");
 
 	if (posix_memalign(&area, page_size, page_size))
@@ -385,8 +391,6 @@ static void set_test_type(const char *type)
 
 static void parse_test_type_arg(const char *raw_type)
 {
-	uint64_t features = UFFD_API_FEATURES;
-
 	set_test_type(raw_type);
 
 	if (!test_type)
@@ -409,8 +413,8 @@ static void parse_test_type_arg(const char *raw_type)
 	 * feature.
 	 */
 
-	if (userfaultfd_open(&features))
-		err("Userfaultfd open failed");
+	if (uffd_get_features(&features))
+		err("failed to get available features");
 
 	test_uffdio_wp = test_uffdio_wp &&
 		(features & UFFD_FEATURE_PAGEFAULT_FLAG_WP);
-- 
2.44.0

Re: [PATCH v2 2/3] Update uffd-stress to handle EINVAL for unset config features

[Peter Xu]

On Fri, Jun 21, 2024 at 02:12:23PM -0400, Audra Mitchell wrote:
> Now that we have updated userfaultfd_api to correctly return
> EIVAL when a feature is requested but not available, let's fix
> the uffd-stress test to only set the UFFD_FEATURE_WP_UNPOPULATED
> feature when the config is set. In addition, still run the test if
> the CONFIG_PTE_MARKER_UFFD_WP is not set, just dont use the corresponding
> UFFD_FEATURE_WP_UNPOPULATED feature.
> 
> Signed-off-by: Audra Mitchell <audra@redhat.com>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v2 3/3] Turn off test_uffdio_wp if CONFIG_PTE_MARKER_UFFD_WP is not configured.

[Audra Mitchell]

If CONFIG_PTE_MARKER_UFFD_WP is disabled, then testing with test_uffdio_up
enables calling uffdio_regsiter with the flag UFFDIO_REGISTER_MODE_WP. The
kernel ensures in vma_can_userfault() that if CONFIG_PTE_MARKER_UFFD_WP
is disabled, only allow the VM_UFFD_WP on anonymous vmas.

Signed-off-by: Audra Mitchell <audra@redhat.com>
---
 tools/testing/selftests/mm/uffd-stress.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
index b9b6d858eab8..2601c9dfadd6 100644
--- a/tools/testing/selftests/mm/uffd-stress.c
+++ b/tools/testing/selftests/mm/uffd-stress.c
@@ -419,6 +419,9 @@ static void parse_test_type_arg(const char *raw_type)
 	test_uffdio_wp = test_uffdio_wp &&
 		(features & UFFD_FEATURE_PAGEFAULT_FLAG_WP);
 
+	if (test_type != TEST_ANON && !(features & UFFD_FEATURE_WP_UNPOPULATED))
+		test_uffdio_wp = false;
+
 	close(uffd);
 	uffd = -1;
 }
-- 
2.44.0

Re: [PATCH v2 3/3] Turn off test_uffdio_wp if CONFIG_PTE_MARKER_UFFD_WP is not configured.

[Peter Xu]

On Fri, Jun 21, 2024 at 02:12:24PM -0400, Audra Mitchell wrote:
> If CONFIG_PTE_MARKER_UFFD_WP is disabled, then testing with test_uffdio_up

Here you're talking about pte markers, then..

> enables calling uffdio_regsiter with the flag UFFDIO_REGISTER_MODE_WP. The
> kernel ensures in vma_can_userfault() that if CONFIG_PTE_MARKER_UFFD_WP
> is disabled, only allow the VM_UFFD_WP on anonymous vmas.
> 
> Signed-off-by: Audra Mitchell <audra@redhat.com>
> ---
>  tools/testing/selftests/mm/uffd-stress.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
> index b9b6d858eab8..2601c9dfadd6 100644
> --- a/tools/testing/selftests/mm/uffd-stress.c
> +++ b/tools/testing/selftests/mm/uffd-stress.c
> @@ -419,6 +419,9 @@ static void parse_test_type_arg(const char *raw_type)
>  	test_uffdio_wp = test_uffdio_wp &&
>  		(features & UFFD_FEATURE_PAGEFAULT_FLAG_WP);
>  
> +	if (test_type != TEST_ANON && !(features & UFFD_FEATURE_WP_UNPOPULATED))
> +		test_uffdio_wp = false;

... here you're checking against wp_unpopulated.  I'm slightly confused.

Are you running this test over shmem/hugetlb when the WP feature isn't
supported?

I'm wondering whether you're looking for UFFD_FEATURE_WP_HUGETLBFS_SHMEM
instead.

Thanks,

> +
>  	close(uffd);
>  	uffd = -1;
>  }
> -- 
> 2.44.0
> 

-- 
Peter Xu

Re: [PATCH v2 3/3] Turn off test_uffdio_wp if CONFIG_PTE_MARKER_UFFD_WP is not configured.

[Audra Mitchell]

On Fri, Jun 21, 2024 at 05:27:43PM -0400, Peter Xu wrote:
> On Fri, Jun 21, 2024 at 02:12:24PM -0400, Audra Mitchell wrote:
> > If CONFIG_PTE_MARKER_UFFD_WP is disabled, then testing with test_uffdio_up
> 
> Here you're talking about pte markers, then..
> 
> > enables calling uffdio_regsiter with the flag UFFDIO_REGISTER_MODE_WP. The
> > kernel ensures in vma_can_userfault() that if CONFIG_PTE_MARKER_UFFD_WP
> > is disabled, only allow the VM_UFFD_WP on anonymous vmas.
> > 
> > Signed-off-by: Audra Mitchell <audra@redhat.com>
> > ---
> >  tools/testing/selftests/mm/uffd-stress.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
> > index b9b6d858eab8..2601c9dfadd6 100644
> > --- a/tools/testing/selftests/mm/uffd-stress.c
> > +++ b/tools/testing/selftests/mm/uffd-stress.c
> > @@ -419,6 +419,9 @@ static void parse_test_type_arg(const char *raw_type)
> >  	test_uffdio_wp = test_uffdio_wp &&
> >  		(features & UFFD_FEATURE_PAGEFAULT_FLAG_WP);
> >  
> > +	if (test_type != TEST_ANON && !(features & UFFD_FEATURE_WP_UNPOPULATED))
> > +		test_uffdio_wp = false;
> 
> ... here you're checking against wp_unpopulated.  I'm slightly confused.
> 
> Are you running this test over shmem/hugetlb when the WP feature isn't
> supported?
>
> I'm wondering whether you're looking for UFFD_FEATURE_WP_HUGETLBFS_SHMEM
> instead.

I can confirm, its all really confusing... So in userfaultfd_api, we disable
three features if CONFIG_PTE_MARKER_UFFD_WP is not enabled- including 
UFFD_FEATURE_WP_UNPOPULATED:

#ifndef CONFIG_PTE_MARKER_UFFD_WP
        uffdio_api.features &= ~UFFD_FEATURE_WP_HUGETLBFS_SHMEM;
        uffdio_api.features &= ~UFFD_FEATURE_WP_UNPOPULATED;
        uffdio_api.features &= ~UFFD_FEATURE_WP_ASYNC;
#endif

If you run the userfaultfd selftests with the run_vmtests script we get
several failures stemming from trying to call uffdio_regsiter with the flag 
UFFDIO_REGISTER_MODE_WP. However, the kernel ensures in vma_can_userfault() 
that if CONFIG_PTE_MARKER_UFFD_WP is disabled, only allow the VM_UFFD_WP -
which is set when you pass the UFFDIO_REGISTER_MODE_WP flag - on 
anonymous vmas.

In parse_test_type_arg() I added the features check against 
UFFD_FEATURE_WP_UNPOPULATED as it seemed the most well know feature/flag. I'm 
more than happy to take any suggestions and adapt them if you have any! 

Thanks in advance and happy Monday!

-- Audra


 
> Thanks,
> 
> > +
> >  	close(uffd);
> >  	uffd = -1;
> >  }
> > -- 
> > 2.44.0
> > 
> 
> -- 
> Peter Xu
> 

Re: [PATCH v2 3/3] Turn off test_uffdio_wp if CONFIG_PTE_MARKER_UFFD_WP is not configured.

[Peter Xu]

On Mon, Jun 24, 2024 at 09:53:57AM -0400, Audra Mitchell wrote:
> On Fri, Jun 21, 2024 at 05:27:43PM -0400, Peter Xu wrote:
> > On Fri, Jun 21, 2024 at 02:12:24PM -0400, Audra Mitchell wrote:
> > > If CONFIG_PTE_MARKER_UFFD_WP is disabled, then testing with test_uffdio_up
> > 
> > Here you're talking about pte markers, then..
> > 
> > > enables calling uffdio_regsiter with the flag UFFDIO_REGISTER_MODE_WP. The
> > > kernel ensures in vma_can_userfault() that if CONFIG_PTE_MARKER_UFFD_WP
> > > is disabled, only allow the VM_UFFD_WP on anonymous vmas.
> > > 
> > > Signed-off-by: Audra Mitchell <audra@redhat.com>
> > > ---
> > >  tools/testing/selftests/mm/uffd-stress.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
> > > index b9b6d858eab8..2601c9dfadd6 100644
> > > --- a/tools/testing/selftests/mm/uffd-stress.c
> > > +++ b/tools/testing/selftests/mm/uffd-stress.c
> > > @@ -419,6 +419,9 @@ static void parse_test_type_arg(const char *raw_type)
> > >  	test_uffdio_wp = test_uffdio_wp &&
> > >  		(features & UFFD_FEATURE_PAGEFAULT_FLAG_WP);
> > >  
> > > +	if (test_type != TEST_ANON && !(features & UFFD_FEATURE_WP_UNPOPULATED))
> > > +		test_uffdio_wp = false;
> > 
> > ... here you're checking against wp_unpopulated.  I'm slightly confused.
> > 
> > Are you running this test over shmem/hugetlb when the WP feature isn't
> > supported?
> >
> > I'm wondering whether you're looking for UFFD_FEATURE_WP_HUGETLBFS_SHMEM
> > instead.
> 
> I can confirm, its all really confusing... So in userfaultfd_api, we disable
> three features if CONFIG_PTE_MARKER_UFFD_WP is not enabled- including 
> UFFD_FEATURE_WP_UNPOPULATED:
> 
> #ifndef CONFIG_PTE_MARKER_UFFD_WP
>         uffdio_api.features &= ~UFFD_FEATURE_WP_HUGETLBFS_SHMEM;
>         uffdio_api.features &= ~UFFD_FEATURE_WP_UNPOPULATED;
>         uffdio_api.features &= ~UFFD_FEATURE_WP_ASYNC;
> #endif
> 
> If you run the userfaultfd selftests with the run_vmtests script we get
> several failures stemming from trying to call uffdio_regsiter with the flag 
> UFFDIO_REGISTER_MODE_WP. However, the kernel ensures in vma_can_userfault() 
> that if CONFIG_PTE_MARKER_UFFD_WP is disabled, only allow the VM_UFFD_WP -
> which is set when you pass the UFFDIO_REGISTER_MODE_WP flag - on 
> anonymous vmas.
> 
> In parse_test_type_arg() I added the features check against 
> UFFD_FEATURE_WP_UNPOPULATED as it seemed the most well know feature/flag. I'm 
> more than happy to take any suggestions and adapt them if you have any! 

There're documents for these features in the headers:

	 * UFFD_FEATURE_WP_HUGETLBFS_SHMEM indicates that userfaultfd
	 * write-protection mode is supported on both shmem and hugetlbfs.
	 *
	 * UFFD_FEATURE_WP_UNPOPULATED indicates that userfaultfd
	 * write-protection mode will always apply to unpopulated pages
	 * (i.e. empty ptes).  This will be the default behavior for shmem
	 * & hugetlbfs, so this flag only affects anonymous memory behavior
	 * when userfault write-protection mode is registered.

While in this context ("test_type != TEST_ANON") IIUC the accurate feature
to check is UFFD_FEATURE_WP_HUGETLBFS_SHMEM.

In most kernels they should behave the same indeed, but note that since
UNPOPULATED was introduced later than shmem/hugetlb support, it means on
some kernel the result of checking these two features will be different.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 3/3] Turn off test_uffdio_wp if CONFIG_PTE_MARKER_UFFD_WP is not configured.

[Andrew Morton]

On Mon, 24 Jun 2024 10:42:00 -0400 Peter Xu <peterx@redhat.com> wrote:

> >         uffdio_api.features &= ~UFFD_FEATURE_WP_HUGETLBFS_SHMEM;
> >         uffdio_api.features &= ~UFFD_FEATURE_WP_UNPOPULATED;
> >         uffdio_api.features &= ~UFFD_FEATURE_WP_ASYNC;
> > #endif
> > 
> > If you run the userfaultfd selftests with the run_vmtests script we get
> > several failures stemming from trying to call uffdio_regsiter with the flag 
> > UFFDIO_REGISTER_MODE_WP. However, the kernel ensures in vma_can_userfault() 
> > that if CONFIG_PTE_MARKER_UFFD_WP is disabled, only allow the VM_UFFD_WP -
> > which is set when you pass the UFFDIO_REGISTER_MODE_WP flag - on 
> > anonymous vmas.
> > 
> > In parse_test_type_arg() I added the features check against 
> > UFFD_FEATURE_WP_UNPOPULATED as it seemed the most well know feature/flag. I'm 
> > more than happy to take any suggestions and adapt them if you have any! 
> 
> There're documents for these features in the headers:
> 
> 	 * UFFD_FEATURE_WP_HUGETLBFS_SHMEM indicates that userfaultfd
> 	 * write-protection mode is supported on both shmem and hugetlbfs.
> 	 *
> 	 * UFFD_FEATURE_WP_UNPOPULATED indicates that userfaultfd
> 	 * write-protection mode will always apply to unpopulated pages
> 	 * (i.e. empty ptes).  This will be the default behavior for shmem
> 	 * & hugetlbfs, so this flag only affects anonymous memory behavior
> 	 * when userfault write-protection mode is registered.
> 
> While in this context ("test_type != TEST_ANON") IIUC the accurate feature
> to check is UFFD_FEATURE_WP_HUGETLBFS_SHMEM.
> 
> In most kernels they should behave the same indeed, but note that since
> UNPOPULATED was introduced later than shmem/hugetlb support, it means on
> some kernel the result of checking these two features will be different.

I'm unsure what to do with this series.  Peter, your review comments
are unclear - do you request updates?

Also, the patches weren't cc:linux-mm which limits the audience.  I'll
drop this version.  Audra, please continue to move this forward.

Re: [PATCH v2 3/3] Turn off test_uffdio_wp if CONFIG_PTE_MARKER_UFFD_WP is not configured.

[Peter Xu]

On Tue, Jun 25, 2024 at 04:05:58PM -0700, Andrew Morton wrote:
> On Mon, 24 Jun 2024 10:42:00 -0400 Peter Xu <peterx@redhat.com> wrote:
> 
> > >         uffdio_api.features &= ~UFFD_FEATURE_WP_HUGETLBFS_SHMEM;
> > >         uffdio_api.features &= ~UFFD_FEATURE_WP_UNPOPULATED;
> > >         uffdio_api.features &= ~UFFD_FEATURE_WP_ASYNC;
> > > #endif
> > > 
> > > If you run the userfaultfd selftests with the run_vmtests script we get
> > > several failures stemming from trying to call uffdio_regsiter with the flag 
> > > UFFDIO_REGISTER_MODE_WP. However, the kernel ensures in vma_can_userfault() 
> > > that if CONFIG_PTE_MARKER_UFFD_WP is disabled, only allow the VM_UFFD_WP -
> > > which is set when you pass the UFFDIO_REGISTER_MODE_WP flag - on 
> > > anonymous vmas.
> > > 
> > > In parse_test_type_arg() I added the features check against 
> > > UFFD_FEATURE_WP_UNPOPULATED as it seemed the most well know feature/flag. I'm 
> > > more than happy to take any suggestions and adapt them if you have any! 
> > 
> > There're documents for these features in the headers:
> > 
> > 	 * UFFD_FEATURE_WP_HUGETLBFS_SHMEM indicates that userfaultfd
> > 	 * write-protection mode is supported on both shmem and hugetlbfs.
> > 	 *
> > 	 * UFFD_FEATURE_WP_UNPOPULATED indicates that userfaultfd
> > 	 * write-protection mode will always apply to unpopulated pages
> > 	 * (i.e. empty ptes).  This will be the default behavior for shmem
> > 	 * & hugetlbfs, so this flag only affects anonymous memory behavior
> > 	 * when userfault write-protection mode is registered.
> > 
> > While in this context ("test_type != TEST_ANON") IIUC the accurate feature
> > to check is UFFD_FEATURE_WP_HUGETLBFS_SHMEM.
> > 
> > In most kernels they should behave the same indeed, but note that since
> > UNPOPULATED was introduced later than shmem/hugetlb support, it means on
> > some kernel the result of checking these two features will be different.
> 
> I'm unsure what to do with this series.  Peter, your review comments
> are unclear - do you request updates?

Yes, or some clarification from Audra would also work.

What I was trying to say is here I think the code should check against
UFFD_FEATURE_WP_HUGETLBFS_SHMEM instead.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 3/3] Turn off test_uffdio_wp if CONFIG_PTE_MARKER_UFFD_WP is not configured.

[Audra Mitchell]

On Tue, Jun 25, 2024 at 07:55:14PM -0400, Peter Xu wrote:
> On Tue, Jun 25, 2024 at 04:05:58PM -0700, Andrew Morton wrote:
> > On Mon, 24 Jun 2024 10:42:00 -0400 Peter Xu <peterx@redhat.com> wrote:
> > 
> > > >         uffdio_api.features &= ~UFFD_FEATURE_WP_HUGETLBFS_SHMEM;
> > > >         uffdio_api.features &= ~UFFD_FEATURE_WP_UNPOPULATED;
> > > >         uffdio_api.features &= ~UFFD_FEATURE_WP_ASYNC;
> > > > #endif
> > > > 
> > > > If you run the userfaultfd selftests with the run_vmtests script we get
> > > > several failures stemming from trying to call uffdio_regsiter with the flag 
> > > > UFFDIO_REGISTER_MODE_WP. However, the kernel ensures in vma_can_userfault() 
> > > > that if CONFIG_PTE_MARKER_UFFD_WP is disabled, only allow the VM_UFFD_WP -
> > > > which is set when you pass the UFFDIO_REGISTER_MODE_WP flag - on 
> > > > anonymous vmas.
> > > > 
> > > > In parse_test_type_arg() I added the features check against 
> > > > UFFD_FEATURE_WP_UNPOPULATED as it seemed the most well know feature/flag. I'm 
> > > > more than happy to take any suggestions and adapt them if you have any! 
> > > 
> > > There're documents for these features in the headers:
> > > 
> > > 	 * UFFD_FEATURE_WP_HUGETLBFS_SHMEM indicates that userfaultfd
> > > 	 * write-protection mode is supported on both shmem and hugetlbfs.
> > > 	 *
> > > 	 * UFFD_FEATURE_WP_UNPOPULATED indicates that userfaultfd
> > > 	 * write-protection mode will always apply to unpopulated pages
> > > 	 * (i.e. empty ptes).  This will be the default behavior for shmem
> > > 	 * & hugetlbfs, so this flag only affects anonymous memory behavior
> > > 	 * when userfault write-protection mode is registered.
> > > 
> > > While in this context ("test_type != TEST_ANON") IIUC the accurate feature
> > > to check is UFFD_FEATURE_WP_HUGETLBFS_SHMEM.
> > > 
> > > In most kernels they should behave the same indeed, but note that since
> > > UNPOPULATED was introduced later than shmem/hugetlb support, it means on
> > > some kernel the result of checking these two features will be different.
> > 
> > I'm unsure what to do with this series.  Peter, your review comments
> > are unclear - do you request updates?
> 
> Yes, or some clarification from Audra would also work.
> 
> What I was trying to say is here I think the code should check against
> UFFD_FEATURE_WP_HUGETLBFS_SHMEM instead.

I was meaning to reply back and ask if Andrew wanted me to push a v3 and
change the check from UFFD_FEATURE_WP_UNPOPULATED to 
UFFD_FEATURE_WP_HUGETLBFS_SHMEM or if he just wanted to do it, but I'll go
ahead and submit v3 with the change shortly. 

Also as an aside I ran scripts/get_maintainer.pl to get the email list. I
probably should have thought a little bit about why the linux-mm list was
missing....

Sorry about the delay and confusion!

 
> Thanks,
> 
> -- 
> Peter Xu
> 

Re: [PATCH v2 1/3] Fix userfaultfd_api to return EINVAL as expected

[Peter Xu]

On Fri, Jun 21, 2024 at 02:12:22PM -0400, Audra Mitchell wrote:
> Currently if we request a feature that is not set in the Kernel
> config we fail silently and return all the available features. However,
> the man page indicates we should return an EINVAL.
> 
> We need to fix this issue since we can end up with a Kernel warning
> should a program request the feature UFFD_FEATURE_WP_UNPOPULATED on
> a kernel with the config not set with this feature.
> 
>  [  200.812896] WARNING: CPU: 91 PID: 13634 at mm/memory.c:1660 zap_pte_range+0x43d/0x660
>  [  200.820738] Modules linked in:
>  [  200.869387] CPU: 91 PID: 13634 Comm: userfaultfd Kdump: loaded Not tainted 6.9.0-rc5+ #8
>  [  200.877477] Hardware name: Dell Inc. PowerEdge R6525/0N7YGH, BIOS 2.7.3 03/30/2022
>  [  200.885052] RIP: 0010:zap_pte_range+0x43d/0x660
> 
> Fixes: e06f1e1dd499 ("userfaultfd: wp: enabled write protection in userfaultfd API")
> Signed-off-by: Audra Mitchell <audra@redhat.com>

Please prefix the subject with "mm/uffd:" or "mm/userfault:" if there's a
repost.

Acked-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

Re: [PATCH v2 1/3] Fix userfaultfd_api to return EINVAL as expected

[Andrew Morton]

On Fri, 21 Jun 2024 14:12:22 -0400 Audra Mitchell <audra@redhat.com> wrote:

> Currently if we request a feature that is not set in the Kernel
> config we fail silently and return all the available features. However,
> the man page indicates we should return an EINVAL.
> 
> We need to fix this issue since we can end up with a Kernel warning
> should a program request the feature UFFD_FEATURE_WP_UNPOPULATED on
> a kernel with the config not set with this feature.
> 
>  [  200.812896] WARNING: CPU: 91 PID: 13634 at mm/memory.c:1660 zap_pte_range+0x43d/0x660
>  [  200.820738] Modules linked in:
>  [  200.869387] CPU: 91 PID: 13634 Comm: userfaultfd Kdump: loaded Not tainted 6.9.0-rc5+ #8
>  [  200.877477] Hardware name: Dell Inc. PowerEdge R6525/0N7YGH, BIOS 2.7.3 03/30/2022
>  [  200.885052] RIP: 0010:zap_pte_range+0x43d/0x660
> 
> Fixes: e06f1e1dd499 ("userfaultfd: wp: enabled write protection in userfaultfd API")

A userspace-triggerable WARN is bad.  I added cc:stable to this.

Re: [PATCH v2 1/3] Fix userfaultfd_api to return EINVAL as expected

[Peter Xu]

On Fri, Jun 21, 2024 at 06:03:30PM -0700, Andrew Morton wrote:
> On Fri, 21 Jun 2024 14:12:22 -0400 Audra Mitchell <audra@redhat.com> wrote:
> 
> > Currently if we request a feature that is not set in the Kernel
> > config we fail silently and return all the available features. However,
> > the man page indicates we should return an EINVAL.
> > 
> > We need to fix this issue since we can end up with a Kernel warning
> > should a program request the feature UFFD_FEATURE_WP_UNPOPULATED on
> > a kernel with the config not set with this feature.
> > 
> >  [  200.812896] WARNING: CPU: 91 PID: 13634 at mm/memory.c:1660 zap_pte_range+0x43d/0x660
> >  [  200.820738] Modules linked in:
> >  [  200.869387] CPU: 91 PID: 13634 Comm: userfaultfd Kdump: loaded Not tainted 6.9.0-rc5+ #8
> >  [  200.877477] Hardware name: Dell Inc. PowerEdge R6525/0N7YGH, BIOS 2.7.3 03/30/2022
> >  [  200.885052] RIP: 0010:zap_pte_range+0x43d/0x660
> > 
> > Fixes: e06f1e1dd499 ("userfaultfd: wp: enabled write protection in userfaultfd API")
> 
> A userspace-triggerable WARN is bad.  I added cc:stable to this.

Andrew,

Note that this change may fix a WARN, but it may also start to break
userspace which might be worse if it happens, IMHO.  I forgot to mention
that here, but only mentioned that in v1, and from that POV not copying
stable seems the right thing.

https://lore.kernel.org/all/ZjuIEH8TW2tWcqXQ@x1n/

        In summary: I think we can stick with Fixes on e06f1e1dd499, but we
        don't copy stable.  The major reason we don't copy stable here is
        not only about complexity of such backport, but also that there can
        be apps trying to pass in unsupported bits (even if the kernel
        didn't support it) but keep using MISSING mode only, then we
        shouldn't fail them easily after a stable upgrade.  Just smells
        dangerous to backport.

I'm not sure what's the best solution for this.  A sweet spot might be that
we start to fix it in new kernels only but not stable ones.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 1/3] Fix userfaultfd_api to return EINVAL as expected

[Andrew Morton]

On Sun, 23 Jun 2024 11:26:37 -0400 Peter Xu <peterx@redhat.com> wrote:

> > > Fixes: e06f1e1dd499 ("userfaultfd: wp: enabled write protection in userfaultfd API")
> > 
> > A userspace-triggerable WARN is bad.  I added cc:stable to this.
> 
> Andrew,
> 
> Note that this change may fix a WARN, but it may also start to break
> userspace which might be worse if it happens, IMHO.  I forgot to mention
> that here, but only mentioned that in v1, and from that POV not copying
> stable seems the right thing.
> 
> https://lore.kernel.org/all/ZjuIEH8TW2tWcqXQ@x1n/
> 
>         In summary: I think we can stick with Fixes on e06f1e1dd499, but we
>         don't copy stable.  The major reason we don't copy stable here is
>         not only about complexity of such backport, but also that there can
>         be apps trying to pass in unsupported bits (even if the kernel
>         didn't support it) but keep using MISSING mode only, then we
>         shouldn't fail them easily after a stable upgrade.  Just smells
>         dangerous to backport.

OK.  And I'll move it into the 6.11-rc1 queue, for the next merge window.