Message-ID: <20201203180234.GJ108496@xz-x1>
Date:   Thu, 3 Dec 2020 13:02:34 -0500
From:   Peter Xu <peterx@...hat.com>
To:     Hugh Dickins <hughd@...gle.com>
Cc:     Andrea Arcangeli <aarcange@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        David Hildenbrand <david@...hat.com>
Subject: Re: [PATCH v2] mm: Don't fault around userfaultfd-registered regions
 on reads

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.
> > > 
> > > Search for non_swap_entry and for migration_entry, to find places that 
> > > might need to learn about this new variety.
> > > 
> > > 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.
> > > 
> > > Note that fork's copy_page_range() does not "copy ptes where a page
> > > fault will fill them correctly", so would in effect put a pte_none
> > > into the child where the parent has this uffd_wp entry.  I don't know
> > > anything about uffd versus fork, whether that would pose a problem.
> > 
> > Thanks for the idea, Hugh!
> > 
> > I thought about something similar today, but instead of swap entries, I was
> > thinking about constantly filling in a pte with a value of "_PAGE_PROTNONE |
> > _PAGE_UFFD_WP" when e.g. we'd like to zap a page with shmem+uffd-wp. I feel
> > like the fundamental idea is similar - we can somehow keep the pte with uffd-wp
> > information even if zapped/swapped-out, so as long as the shmem access will
> > fruther trap into the fault handler, then we can operate on that pte and read
> > that information out, like recover that pte into a normal pte (with swap/page
> > cache, and vma/addr information, we'll be able to) and then we can retry the
> > fault.
> 
> Yes, I think that should work too: I can't predict which way would cause
> less trouble.
> 
> We usually tend to keep away from protnone games, because NUMA balancing
> use of protnone is already confusing enough.

Yes it is tricky.  However it gives me the feeling that numa balancing and its
protnone trick provided a general solution for things like this, so that we can
trap a per-mm per-pte page access like this.

With that, I'm currently slightly prefer to try the protnone way first, because
using swp entry could be a bit misleading from the 1st glance - note that when
this happens, we could have two states for this pte:

  1. Page in shmem page cache
  2. Page in shmem swap cache (so page cache is a value)

And actually there's another 3rd state that should never happen as long as the
userspace does unprotect properly, but I guess we'd better also take care of:

  3. Page is not cached at all (page missing; logically should not happen
     because when page cache evicted then all ptes should have been removed,
     however since this pte is not linked to the page in any way, it could get
     lost?  Then we should simply retry the fault after recovering the tricky
     pte into an all-zero pte)

It'll be a bit odd imho to use a swp entry to represent all these states, for
example we can see the pte is a swp-like entry but in reality the shmem page
sits right in the page cache..

So I'm thinking whether we could decouple the pte_protnone() idea with numa
balancing first (currently, there's a close bind), let numa depend on protnone,
then uffd-wp+shmem can also depend on protnone.

> 
> But those ptes will be pte_present(), so you must provide a pfn,

Could I ask why?

> and I think if you use the zero_pfn, vm_normal_page() will return false on it,
> and avoid special casing (and reference counting) it in various places.

I'll keep this in mind, thanks.

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?

-- 
Peter Xu

Powered by blists - more mailing lists