linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Peter Xu <peterx@redhat.com>
To: Andrea Arcangeli <aarcange@redhat.com>, Hugh Dickins <hughd@google.com>
Cc: Hugh Dickins <hughd@google.com>,
	Matthew Wilcox <willy@infradead.org>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Mike Rapoport <rppt@linux.vnet.ibm.com>,
	David Hildenbrand <david@redhat.com>
Subject: Re: [PATCH v2] mm: Don't fault around userfaultfd-registered regions on reads
Date: Thu, 3 Dec 2020 21:30:51 -0500	[thread overview]
Message-ID: <20201204023051.GL108496@xz-x1> (raw)
In-Reply-To: <X8lADgHCRqlQi3Xa@redhat.com>

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


  reply	other threads:[~2020-12-04  2:32 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-30 23:06 Peter Xu
2020-12-01  9:30 ` David Hildenbrand
2020-12-01 12:59 ` Matthew Wilcox
2020-12-01 22:30   ` Peter Xu
2020-12-02  0:02     ` Andrea Arcangeli
2020-12-02 22:37       ` Hugh Dickins
2020-12-02 23:41         ` Peter Xu
2020-12-03  5:36           ` Hugh Dickins
2020-12-03 18:02             ` Peter Xu
2020-12-03 19:44               ` Andrea Arcangeli
2020-12-04  2:30                 ` Peter Xu [this message]
2020-12-04  4:10                   ` Andrea Arcangeli
2020-12-04  5:59                     ` Hugh Dickins
2020-12-04 16:50                       ` Peter Xu
2020-12-04 18:12                     ` Andrea Arcangeli
2020-12-04 19:23                       ` Peter Xu
2020-12-04 19:37                         ` Andrea Arcangeli
2020-12-04 20:21                           ` Peter Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201204023051.GL108496@xz-x1 \
    --to=peterx@redhat.com \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=david@redhat.com \
    --cc=hughd@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=rppt@linux.vnet.ibm.com \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH v2] mm: Don'\''t fault around userfaultfd-registered regions on reads' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).