Messages in this thread | | | Date | Tue, 15 Nov 2022 18:54:09 +0100 | Subject | Re: [PATCH v2 1/2] mm/migrate: Fix read-only page got writable when recover pte | From | David Hildenbrand <> |
| |
On 15.11.22 18:22, David Hildenbrand wrote: >>> 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".
Forgot to reply to that part:
No it isn't natural. But sneaking such a change into your fix seems wrong. Touching !uffd-wp code should be separate, if we want to do this at all (as we discussed, maybe the better/cleaner approach is to eliminate writable migration entries if possible).
-- Thanks,
David / dhildenb
| |