Re: [PATCH v2] mm/uffd: UFFD_FEATURE_WP_UNPOPULATED

[David Hildenbrand]

On 01.03.23 18:13, Muhammad Usama Anjum wrote:
> On 3/1/23 8:19 PM, Peter Xu wrote:
> > On Wed, Mar 01, 2023 at 12:55:51PM +0500, Muhammad Usama Anjum wrote:
> > > Hi Peter,
> >
> > Hi, Muhammad,
> >
> > > While using WP_UNPOPULATED, we get stuck if newly allocated memory is read
> > > without initialization. This can be reproduced by either of the following
> > > statements:
> > >      printf("%c", buffer[0]);
> > >      buffer[0]++;
> > >
> > > This bug has start to appear on this patch. How are you handling reading
> > > newly allocated memory when WP_UNPOPULATED is defined?
> >
> > Yes it's a bug, thanks for the reproducer. You're right I missed a trivial
> > but important detail.  Could you try apply below on top?
> >
> > ---8<---
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 46934133bd0b..2f4b3892948b 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -4062,7 +4062,7 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
> >                                                  vma->vm_page_prot));
> >                  vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
> >                                  vmf->address, &vmf->ptl);
> > -               if (!pte_none(*vmf->pte)) {
> > +               if (vmf_pte_changed(vmf)) {
> >                          update_mmu_tlb(vma, vmf->address, vmf->pte);
> >                          goto unlock;
> >                  }
> > ---8<---
> This patch works. Thank you so much!
>
> > I can send a new version after you confirmed it at least works on your
> > side. I'll also add some more test to cover that in the next version.
> >
> > The current smoke test within this patch is really light; I somehow rely on
> > you on this patch on the testing side, and thanks for that.
> >
> > > Running my pagemap_ioctl selftest as benchmark in a VM:
> > > without zeropage / wp_unpopulated (decide from pte_none() if page is dirty
> > > or not, buggy and wrong implementation, just for reference)
> > > 26.608 seconds
> > > with zeropage
> > > 39.203 seconds
> > > with wp_unpopulated
> > > 62.907 seconds
> > >
> > > 136% worse performance overall
> > > 60% worse performance of unpopulated than zeropage
> >
> > Yes this is unfortunate, because we're protecting more things than before
> > when with WP_ZEROPAGE / WP_UNPOPULATED but that's what it is for (when we
> > want to make sure that accuracy on the holes).
> >
> > I didn't look closer to your whole test suite yet, but my pure test on
> > protection above should mean that it's still much better for such a use
> > case than either (1) pre-read or (2) MADV_POPULATE_READ.
> Ohh... I should stop comparing UNPOPULATE with buggy implementation and
> compare with pre-read. I've compared apples with oranges.

Note that I think there are ways to avoid that overhead (as raised in 
reply to Peter's reply), so IMHO it's still valuable to know which 
benefit we could have without allocating pagetables and placing the 
shared zeropages.

> I'll do better benchmark for the comparison sake. I'll let you know if the
> performance is becoming an issue. Overall we need pagemap_ioctl + UFFD to
> correctly emulate Windows syscall. Secondly we also need good performance
> (more the better).

I'm curious, are you eventually applying UFFD to possibly large (sparse) 
VMAs, that are eventually even only PROT_READ or PROT_WRITE?

Especially for such large sparse VMAs, the current way of allocating 
pagetables to place markers/zeropages is far from optimal.

--
Thanks,

David / dhildenb