Message-ID: <Y3O5bCXSbvKJrjRL@x1n>
Date:   Tue, 15 Nov 2022 11:08:12 -0500
From:   Peter Xu <peterx@...hat.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Andrea Arcangeli <aarcange@...hat.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Ives van Hoorne <ives@...esandbox.io>,
        Nadav Amit <nadav.amit@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>, stable@...r.kernel.org
Subject: Re: [PATCH v2 1/2] mm/migrate: Fix read-only page got writable when
 recover pte

On Tue, Nov 15, 2022 at 10:13:24AM +0100, David Hildenbrand wrote:
> > > 
> > > Any particular reason why not to simply glue this to pte_swp_uffd_wp(),
> > > because only that needs special care:
> > > 
> > > if (pte_swp_uffd_wp(*pvmw.pte)) {
> > > 	pte = pte_wrprotect(pte);
> > > 	pte = pte_mkuffd_wp(pte);
> > > }
> > > 
> > > 
> > > And that would match what actually should have been done in commit
> > > f45ec5ff16a7 -- only special-case uffd-wp.
> > > 
> > > Note that I think there are cases where we have a PTE that was !writable,
> > > but after migration we can map it writable.
> > 
> > The thing is recovering the pte into its original form is the safest
> > approach to me, so I think we need justification on why it's always safe to
> > set the write bit.
> > 
> > Or do you perhaps have solid clue and think it's always safe
> The problem I am having with this broader change, is that this changes
> something independent of your original patch/problem.
> 
> If we identify this to be an actual problem, it should most probably be
> separate fix + backport.
> 
> 
> My understanding is that vma->vm_page_prot always tells you what the default
> PTE protection in a mapping is.
> 
> If the mapping is private, it is never writable (due to COW). Similarly, if
> the shared file mapping needs writenotify, it is never writable.

Right.

> 
> 
> I consider UFFD-wp a special case: while the default VMA protection might
> state that it is writable, you actually want individual PTEs to be
> write-protected and have to manually remove the protection.
> 
> softdirty tracking is another special case: however, softdirty tracking is
> enabled for the whole VMA. For remove_migration_pte() that should be fine (I
> guess) because writenotify is active when the VMA needs to track softdirty
> bits, and consequently vma->vm_page_prot has the proper default permissions.
> 
> 
> I wonder if the following (valid), for example is possible:
> 
> 
> 1) clear_refs() clears VM_SOFTDIRTY and pte_wrprotect() the pte.
> -> writenotify is active and vma->vm_page_prot updated accordingly
> 
> VM_SOFTDIRTY is reset due to VMA merging and vma->vm_page_prot is updated
> accordingly. See mmap_region() where we set VM_SOFTDIRTY.
> 
> If you now migrate the (still write-protected in the PTE) page, it was not
> writable, but it can be writable on the destination.

I didn't even notice merging could work with soft-dirty enabled, that's
interesting to know.

Yes I think it's possible and I agree it's safe, as VM_SOFTDIRTY is set for
the merged vma so afaiu the write bit is safe to set.  We get a bunch of
false positives but that's how soft-dirty works.

I think the whole problem is easier if we see this at a higher level.
You're discussing this from vma pov and it's fair to do so, at least I
agree with what you mentioned so far and I can't see anything outside
uffd-wp that can be affected.  However, it is also true when you noticed we
already have quite a few paragraphs trying to discuss the safety for this
and that, that's the part where I think we need justification and it's not
that "natural".

For "natural", I meant fundamentally we're talking about page migrations
here.  The natural way (at least to me) for page migration to happen as a
fundamental rule is that, we leverag the migration pte to make sure the pte
being stable so we can do the migration work, then we "recover" the pte to
present either by a full recovery or just (hopefully) only replace the pfn,
keeping all the rest untouched.

One thing to prove that is we have two migration entries not one (I'm
temporarily put the exclusive read one aside since that's solving different
problems): MIGRATION_READ and MIGRATION_WRITE.  If we only rely on vma
flags logically we don't need MIGRATION_READ and MIGRATION_WRITE, we only
need MIGRATION generic swap pte then we recover the write bit from vma
flags and other things (like uffd-wp, currently we have the bit set in swap
pte besides the swap entry type).

So maybe one day we can use two migration types rather than three
(MIGRATION and MIGRATION_EXCLUSIVE)?  I can't tell, but hopefully that
shows what I meant, that we need further justification to grant write bit
only base on vma, rather than recovering write bit based on migration entry
type.

> 
> > 
> > > 
> > > BTW, does unuse_pte() need similar care?
> > > 
> > > new_pte = pte_mkold(mk_pte(page, vma->vm_page_prot));
> > > if (pte_swp_uffd_wp(*pte))
> > > 	new_pte = pte_mkuffd_wp(new_pte);
> > > set_pte_at(vma->vm_mm, addr, pte, new_pte);
> > 
> > I think unuse path is fine because unuse only applies to private mappings,
> > so we should always have the W bit removed there within mk_pte().
> 
> You're right, however, shmem swapping confuses me. Maybe that does not apply
> here.

Yeah these are confusing.  Actually I think the unuse path should apply to
private mappings on shmem when the page was CoWed already and then got
swapped out, but again as long as it's private mapping I think we don't
have write bit anyway in retval of mk_pte() even if it's shmem.

-- 
Peter Xu

Powered by blists - more mailing lists