Re: [PATCH v2] mm: Don't fault around userfaultfd-registered regions on reads

[Peter Xu]

On Thu, Dec 03, 2020 at 02:44:14PM -0500, Andrea Arcangeli wrote:
> On Thu, Dec 03, 2020 at 01:02:34PM -0500, Peter Xu wrote:
> > On Wed, Dec 02, 2020 at 09:36:45PM -0800, Hugh Dickins wrote:
> > > On Wed, 2 Dec 2020, Peter Xu wrote:
> > > > On Wed, Dec 02, 2020 at 02:37:33PM -0800, Hugh Dickins wrote:
> > > > > On Tue, 1 Dec 2020, Andrea Arcangeli wrote:
> > > > > > 
> > > > > > Any suggestions on how to have the per-vaddr per-mm _PAGE_UFFD_WP bit
> > > > > > survive the pte invalidates in a way that remains associated to a
> > > > > > certain vaddr in a single mm (so it can shoot itself in the foot if it
> > > > > > wants, but it can't interfere with all other mm sharing the shmem
> > > > > > file) would be welcome...
> > > > > 
> > > > > I think it has to be a new variety of swap-like non_swap_entry() pte,
> > > > > see include/linux/swapops.h.  Anything else would be more troublesome.
> 
> Agreed. Solving it by changing the unmapping of the ptes is also some
> trouble but less troublesome than adding new bitmaps to the vma to
> handle in vma_merge/vma_split.
> 
> > > But those ptes will be pte_present(), so you must provide a pfn,
> > 
> > Could I ask why?
> 
> _PAGE_PROTNONE exists only for one reason, so pte_present returns true
> and the page is as good as mapped, the pfn is real and mapped,
> everything is up and running fine except _PAGE_PRESENT is not set in
> the pte. _PAGE_PROTNONE doesn't unmap the pte, it only triggers faults
> on a mapped pte.
> 
> When we set _PAGE_PROTNONE and clear _PAGE_PRESENT atomically, nothing
> changes at all for all pte_present and all other VM common code,
> except now you get page faults.
> 
> So numa hinting faults use that and it's a perfect fit for that,
> because you just want to change nothing, but still be notified on
> access.
> 
> Here instead you need to really unmap the page and lose any pfn or
> page reference and everything along with it, just one bit need to
> survive the unmap: the _PAGE_UFFD_WP bit.
> 
> I tend to agree this needs to work more similarly to the migration
> entry like Hugh suggested or an entirely new mechanism to keep "vma
> specific state" alive, for filebacked mappings that get zapped but
> that have still a vma intact.

Thanks Andrea & Hugh. I think I get the point now.

I guess missed the fact that !pte_present() means it's a swap entry by
definition...  So my previous thoughts on using either _PAGE_PROTNONE or
_PAGE_SPECIAL could be a bit silly because if without pte_present() they'll
just be misunderstood as swp entries, then all the _PAGE_* bits won't make any
sense any more, since they could mean some strange (type, offset) tuple of swp
entries.

So yes, I think we need a swp entry, with some special format.

> 
> The vma removal in munmap() syscall, is then the only point where the
> pte is only allowed to be cleared for good and only then the pte can
> be freed.
> 
> Not even MADV_DONTNEED should be allowed to zero out the pte, it can
> drop everything but that single bit.

Right.  Ideally I wouldn't expect any sane userspace that uses uffd-wp with
shmem to MADV_DONTNEED a page right under wr-protection, because it really
shouldn't do that... However from the semantics of MADV_DONTNEED, indeed it
should be the same case as the shmem fallocate code where there's a pre-unmap,
so we should be prepared for that too.

> 
> > Meanwhile, this reminded me another option - besides _PAGE_PROTNONE, can we use
> > _PAGE_SPECIAL?  That sounds better at least from its naming that it tells it's
> > a special page already. We can also leverage existing pte_special() checks here
> > and there, then mimic what we do with pte_devmap(), maybe?
> 
> That's also for mapped pages VM_PFNMAP or similar I think.
> 
> By memory the !pte_present case for filebacked vmas only exists as
> migration entry so I think we'll just add a branch to that case so
> that it can disambiguate the migration entry from the _PAGE_UFFDP_WP
> bit.
> 
> So we can reserve one bit in the migration entry that is always
> enforced zero when it is a migration entry.
> 
> When that bit is set on a non-present page in a filebacked vma, it
> will mean _UFFD_PAGE_WP is set for that vaddr in that mm.

I'm just afraid there's no space left for a migration entry, because migration
entries fills in the pfn information into swp offset field rather than a real
offset (please refer to make_migration_entry())?  I assume PFN can use any bit.
Or did I miss anything?

I went back to see the original proposal from Hugh:

  IIUC you only need a single value, no need to carve out another whole
  swp_type: could probably be swp_offset 0 of any swp_type other than 0.

Hugh/Andrea, sorry if this is a stupid swap question: could you help explain
why swp_offset=0 won't be used by any swap device?  I believe it's correct,
it's just that I failed to figure out the reason myself. :(

Besides above, to reuse the _PAGE_SWP_UFFD_WP (as it's already there), maybe we
can also define the special swp entry as:

  swp_entry(0, _PAGE_SWP_UFFD_WP)

Then as long as we check this against the vma so that we know it's not
anonymous memory, then it's the special pte.  Combined as:

    vma_is_anonymous(vma)==false && swp_entry(0, _PAGE_SWP_UFFD_WP);

> 
> Then we need to pass a parameter in all pte zapping operations to tell
> if the unmap event is happening because the vma has been truncated, or
> if it's happening for any other reason.
> 
> If it's happening for any other reasons (page truncate, MADV_DONTNEED,
> memory pressure to swap/writepage) we just convert any present pte
> with _UFFD_PAGE_WP set, to the non-migration entry non-present pte
> with the reserved migration entry bit set.
> 
> If the present pte has no _UFFD_PAGE_WP then it'll be zapped as usual
> regardless of VM_UFFD_WP in the vma or not.
> 
> If the pte zapping is instead invoked because of a vma truncation, and
> it means it's the last unmap operation on that vaddr, the pte zap
> logic will be told to ignore the _UFFD_PAGE_WP in any present pte so
> the ptes will be zeroed out and later freed as needed.

Agreed.

Thanks,

-- 
Peter Xu