Re: [PATCH RFC] mm/userfaultfd: enable writenotify while userfaultfd-wp is enabled for a VMA

[David Hildenbrand]

On 02.12.22 17:56, David Hildenbrand wrote:
> On 02.12.22 17:33, Peter Xu wrote:
> > On Fri, Dec 02, 2022 at 01:27:48PM +0100, David Hildenbrand wrote:
> > > Currently, we don't enable writenotify when enabling userfaultfd-wp on
> > > a shared writable mapping (for now we only support SHMEM). The consequence
> >
> > and hugetlbfs
> >
> > > is that vma->vm_page_prot will still include write permissions, to be set
> > > as default for all PTEs that get remapped (e.g., mprotect(), NUMA hinting,
> > > page migration, ...).
> >
> > The thing is by default I think we want the write bit..
> >
> > The simple example is (1) register UFFD_WP on shmem writable, (2) write a
> > page.  Here we didn't wr-protect anything, so we want the write bit there.
> >
> > Or the other example is when UFFDIO_COPY with flags==0 even if with
> > VM_UFFD_WP.  We definitely wants the write bit.
> >
> > We only doesn't want the write bit when uffd-wp is explicitly set.
> >
> > I think fundamentally the core is uffd-wp is pte-based, so the information
> > resides in pte not vma.  I'm not strongly objecting this patch, especially
> > you mentioned auto-numa so I need to have a closer look later there.
> > However I do think uffd-wp is slightly special because we always need to
> > consider pte information anyway, so a per-vma information doesn't hugely
> > help, IMHO.
>
> That's the same as softdirty tracking, IMHO.
>
> [...]
>
> > > Running the mprotect() reproducer [1] without this commit:
> > >     $ ./uffd-wp-mprotect
> > >     FAIL: uffd-wp did not fire
> > > Running the mprotect() reproducer with this commit:
> > >     $ ./uffd-wp-mprotect
> > >     PASS: uffd-wp fired
> > >
> > > [1] https://lore.kernel.org/all/222fc0b2-6ec0-98e7-833f-ea868b248446@xxxxxxxxxx/T/#u
> >
> > I still hope for a formal patch (non-rfc) we can have a reproducer outside
> > mprotect().  IMHO mprotect() is really ambiguously here being used with
> > uffd-wp, so not a good example IMO as I explained in the other thread [1].
>
> I took the low hanging fruit to showcase that this is a more generic problem.
> The reproducer is IMHO nice because it's simple and race-free.
>
> > I'll need to off-work most of the rest of today, but maybe I can also have
> > a look in the weekend or Monday more on the numa paths.  Before that, can
> > we first reach a consensus that we have the mm/migrate patch there to be
> > merged first?  These are two issues, IMHO.
> >
> > I know you're against me for some reason, but until now I sincerely don't
> > know why.  That patch sololy recovers write bit status (by removing it for
> > read-only) for a migration entry and that definitely makes sense to me.  As
> > I also mentioned in the old version of that thread, we can rework migration
> > entries and merge READ|WRITE entries into a GENERIC entry one day if you
> > think proper, but that's for later.
>
> I'm not against you. I'm against changing well-working, common code
> when it doesn't make any sense to me to change it. And now we have proof that
> mprotect() just behaves exactly the same way, using the basic rules of vma->vm_page_prot.
>
> Yes, there is broken sparc64 (below), but that shouldn't dictate our implementation.
>
>
> What *would* make sense to me, as I raised, is:
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index dff333593a8a..9fc181fd3c5a 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -213,8 +213,10 @@ static bool remove_migration_pte(struct folio *folio,
>                           pte = pte_mkdirty(pte);
>                   if (is_writable_migration_entry(entry))
>                           pte = maybe_mkwrite(pte, vma);
> -               else if (pte_swp_uffd_wp(*pvmw.pte))
> +               else if (pte_swp_uffd_wp(*pvmw.pte)) {
>                           pte = pte_mkuffd_wp(pte);
> +                       pt = pte_wrprotect(pte);
> +               }
>    
>                   if (folio_test_anon(folio) && !is_readable_migration_entry(entry))
>                           rmap_flags |= RMAP_EXCLUSIVE;
>
>
> It still requires patch each and every possible code location, which I dislike as
> described in the patch description. The fact that there are still uffd-wp bugs
> with your patch makes that hopefully clear. I'd be interested if they can be
> reproduced witht his patch.

And if NUMA hinting is indeed the problem, without this patch what would
be required would most probably be:


diff --git a/mm/memory.c b/mm/memory.c
index 8a6d5c823f91..869d35ef0e24 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4808,6 +4808,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
        pte = pte_mkyoung(pte);
        if (was_writable)
                pte = pte_mkwrite(pte);
+       if (pte_uffd_wp(pte))
+               pte = pte_wrprotect(pte);
        ptep_modify_prot_commit(vma, vmf->address, vmf->pte, old_pte, pte);
        update_mmu_cache(vma, vmf->address, vmf->pte);
        pte_unmap_unlock(vmf->pte, vmf->ptl);


And just to make my point about the migration path clearer: doing it your way
would be:

diff --git a/mm/memory.c b/mm/memory.c
index 8a6d5c823f91..a7c4c1a57f6a 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4808,6 +4808,8 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
        pte = pte_mkyoung(pte);
        if (was_writable)
                pte = pte_mkwrite(pte);
+       else
+               pte = pte_wrprotect(pte);
        ptep_modify_prot_commit(vma, vmf->address, vmf->pte, old_pte, pte);
        update_mmu_cache(vma, vmf->address, vmf->pte);
        pte_unmap_unlock(vmf->pte, vmf->ptl);


And I don't think that's the right approach.


--
Thanks,

David / dhildenb