Messages in this thread | | | Date | Tue, 15 Nov 2022 10:13:24 +0100 | From | David Hildenbrand <> | Subject | Re: [PATCH v2 1/2] mm/migrate: Fix read-only page got writable when recover pte |
| |
>> >> 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.
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.
> >> >> 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.
-- Thanks,
David / dhildenb
| |