| Message ID | 20240313213107.235067-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/memory: Fix missing pte marker for !page on pte zaps | expand |
On 13.03.24 22:31, peterx@redhat.com wrote: > From: Peter Xu <peterx@redhat.com> > > Commit 0cf18e839f64 of large folio zap work broke uffd-wp. Now mm's uffd > unit test "wp-unpopulated" will trigger this WARN_ON_ONCE(). Good that I added the WARN_ON_ONCE() :) > > The WARN_ON_ONCE() asserts that an VMA cannot be registered with > userfaultfd-wp if it contains a !normal page, but it's actually possible. > One example is an anonymous vma, register with uffd-wp, read anything will > install a zero page. Then when zap on it, this should trigger. Are you sure? zap_install_uffd_wp_if_needed() contains right at the start: /* Zap on anonymous always means dropping everything */ if (vma_is_anonymous(vma)) return; So if that's the case the unit test triggers, I'm confused. > > What's more, removing that WARN_ON_ONCE may not be enough either, because > we should also not rely on "whether it's a normal page" to decide whether > pte marker is needed. For example, one can register wr-protect over some > DAX regions to track writes when UFFD_FEATURE_WP_ASYNC enabled, in which > case it can have page==NULL for a devmap but we may want to keep the marker > around. I thought uffd-wp was limited to specific backends only. But looks like that changed with UFFD_FEATURE_WP_ASYNC, I guess? Change itself looks, good. Not sure about the anon_vma example above. Thanks! Acked-by: David Hildenbrand <david@redhat.com> > > Cc: Muhammad Usama Anjum <usama.anjum@collabora.com> > Cc: David Hildenbrand <david@redhat.com> > Fixes: 0cf18e839f64 ("mm/memory: handle !page case in zap_present_pte() separately") > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > mm/memory.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/mm/memory.c b/mm/memory.c > index f2bc6dd15eb8..904f70b99498 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -1536,7 +1536,9 @@ static inline int zap_present_ptes(struct mmu_gather *tlb, > ptep_get_and_clear_full(mm, addr, pte, tlb->fullmm); > arch_check_zapped_pte(vma, ptent); > tlb_remove_tlb_entry(tlb, pte, addr); > - VM_WARN_ON_ONCE(userfaultfd_wp(vma)); > + if (userfaultfd_pte_wp(vma, ptent)) > + zap_install_uffd_wp_if_needed(vma, addr, pte, 1, > + details, ptent); > ksm_might_unmap_zero_page(mm, ptent); > return 1; > }
On 13.03.24 23:03, David Hildenbrand wrote: > On 13.03.24 22:31, peterx@redhat.com wrote: >> From: Peter Xu <peterx@redhat.com> >> >> Commit 0cf18e839f64 of large folio zap work broke uffd-wp. Now mm's uffd >> unit test "wp-unpopulated" will trigger this WARN_ON_ONCE(). > > Good that I added the WARN_ON_ONCE() :) > >> >> The WARN_ON_ONCE() asserts that an VMA cannot be registered with >> userfaultfd-wp if it contains a !normal page, but it's actually possible. >> One example is an anonymous vma, register with uffd-wp, read anything will >> install a zero page. Then when zap on it, this should trigger. > > Are you sure? zap_install_uffd_wp_if_needed() contains right at the start: > > /* Zap on anonymous always means dropping everything */ > if (vma_is_anonymous(vma)) > return; > > So if that's the case the unit test triggers, I'm confused. Ah, got it. It's not that we have to place a marker, just that it can happen. Of course it can. All makes sense. Thanks!
On Wed, Mar 13, 2024 at 11:03:04PM +0100, David Hildenbrand wrote: > On 13.03.24 22:31, peterx@redhat.com wrote: > > From: Peter Xu <peterx@redhat.com> > > > > Commit 0cf18e839f64 of large folio zap work broke uffd-wp. Now mm's uffd > > unit test "wp-unpopulated" will trigger this WARN_ON_ONCE(). > > Good that I added the WARN_ON_ONCE() :) To be explict, VM_WARN_ON_ONCE. :) And that's my guess that you didn't hit it when you posted the series and did the tests, as I know latest distros like Fedora dropped DEBUG_VM, so maybe you had your base config out of there (but I normally have it irrelevant of that). > > > > > The WARN_ON_ONCE() asserts that an VMA cannot be registered with > > userfaultfd-wp if it contains a !normal page, but it's actually possible. > > One example is an anonymous vma, register with uffd-wp, read anything will > > install a zero page. Then when zap on it, this should trigger. > > Are you sure? zap_install_uffd_wp_if_needed() contains right at the start: > > /* Zap on anonymous always means dropping everything */ > if (vma_is_anonymous(vma)) > return; My example is not exactly how the test failed, but should be a simpler version of it. To trigger this warning I don't think it requires the zero page to be wr-protected at all or have any pte marker involved. UFFDIO_REGISTER should suffice, afaiu (feel free to read the example above again; there's no mention of ioctl(UFFDIO_WRITEPROTECT)). > > So if that's the case the unit test triggers, I'm confused. > > > > > What's more, removing that WARN_ON_ONCE may not be enough either, because > > we should also not rely on "whether it's a normal page" to decide whether > > pte marker is needed. For example, one can register wr-protect over some > > DAX regions to track writes when UFFD_FEATURE_WP_ASYNC enabled, in which > > case it can have page==NULL for a devmap but we may want to keep the marker > > around. > > I thought uffd-wp was limited to specific backends only. But looks like that > changed with UFFD_FEATURE_WP_ASYNC, I guess? Correct. That was also what the new PAGEMAP ioctl relies on. > > > Change itself looks, good. Not sure about the anon_vma example above. > > Thanks! > > Acked-by: David Hildenbrand <david@redhat.com> Thanks,
On 13.03.24 23:26, Peter Xu wrote: > On Wed, Mar 13, 2024 at 11:03:04PM +0100, David Hildenbrand wrote: >> On 13.03.24 22:31, peterx@redhat.com wrote: >>> From: Peter Xu <peterx@redhat.com> >>> >>> Commit 0cf18e839f64 of large folio zap work broke uffd-wp. Now mm's uffd >>> unit test "wp-unpopulated" will trigger this WARN_ON_ONCE(). >> >> Good that I added the WARN_ON_ONCE() :) > > To be explict, VM_WARN_ON_ONCE. :) And that's my guess that you didn't hit > it when you posted the series and did the tests, as I know latest distros > like Fedora dropped DEBUG_VM, so maybe you had your base config out of > there (but I normally have it irrelevant of that). It's rather surprising that this went unnoticed for quite a while. I usually do run with DEBUG_VM, but I have a bunch of different VMs where I tests stuff, likely it was off when I ran the selftests.
diff --git a/mm/memory.c b/mm/memory.c index f2bc6dd15eb8..904f70b99498 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -1536,7 +1536,9 @@ static inline int zap_present_ptes(struct mmu_gather *tlb, ptep_get_and_clear_full(mm, addr, pte, tlb->fullmm); arch_check_zapped_pte(vma, ptent); tlb_remove_tlb_entry(tlb, pte, addr); - VM_WARN_ON_ONCE(userfaultfd_wp(vma)); + if (userfaultfd_pte_wp(vma, ptent)) + zap_install_uffd_wp_if_needed(vma, addr, pte, 1, + details, ptent); ksm_might_unmap_zero_page(mm, ptent); return 1; }