Messages in this thread Patches in this message | | | Date | Fri, 2 Dec 2022 18:11:17 +0100 | Subject | Re: [PATCH RFC] mm/userfaultfd: enable writenotify while userfaultfd-wp is enabled for a VMA | From | 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@redhat.com/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
| |