diff mbox series

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

Message ID 20240621181224.3881179-3-audra@redhat.com
State Accepted
Commit a591d35c40237df735e8ffb4792acb6e56bbca48
Headers show
Series [v2,1/3] Fix userfaultfd_api to return EINVAL as expected | expand

Commit Message

Audra Mitchell June 21, 2024, 6:12 p.m. UTC
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(+)

Comments

Peter Xu June 21, 2024, 9:27 p.m. UTC | #1
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
>
Audra Mitchell June 24, 2024, 1:53 p.m. UTC | #2
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
>
Peter Xu June 24, 2024, 2:42 p.m. UTC | #3
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,
Andrew Morton June 25, 2024, 11:05 p.m. UTC | #4
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.
Peter Xu June 25, 2024, 11:55 p.m. UTC | #5
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,
Audra Mitchell June 26, 2024, 12:49 p.m. UTC | #6
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
>
diff mbox series

Patch

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