| 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 |
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 >
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 >
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,
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.
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,
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 --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; }
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(+)