Messages in this thread | | | Date | Mon, 2 Jan 2023 13:29:37 +0100 | From | David Hildenbrand <> | Subject | Re: [PATCH v4 1/3] mm/mprotect: Fix soft-dirty check in can_change_pte_writable() |
| |
On 28.12.22 15:14, Muhammad Usama Anjum wrote: > On 12/19/22 5:19 PM, Muhammad Usama Anjum wrote: >> Addition of vma_soft_dirty_enabled() has tinkered with the soft-dirty PTE >> bit status setting. The internal behavior has changed. The test case was >> shared by David >> (https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com/). >> The explanation is as following: >> >> _Before_ addition of this patch(76aefad628aae), >> m = mmap(2 pages) >> clear_softdirty() >> mremap(m + pag_size) >> mprotect(READ) >> mprotect(READ | WRITE); >> memset(m) >> After memset(), >> PAGE-1 PAGE-2 >> VM_SOFTDIRTY set set >> PTE softdirty flag set set >> /proc//pagemap view set set >> >> >> _After_ addition of this patch(76aefad628aae) >> m = mmap(2 pages) >> clear_softdirty() >> mremap(m + page_size) >> mprotect(READ) >> mprotect(READ | WRITE); >> memset(m) >> After memset(), >> PAGE-1 PAGE-2 >> VM_SOFTDIRTY set set >> PTE softdirty flag *not set* set >> /proc//pagemap view set set >> >> The user's point of view hasn't changed. But internally after this patch, >> the soft-dirty tracking in PTEs gets turn off if VM_SOFTDIRTY is set. The >> soft-dirty tracking in the PTEs shouldn't be just turned off when mprotect >> is used. Why? Because soft-dirty tracking in the PTEs is always enabled >> regardless of VM_SOFTDIRTY is set or not. Example: >> >> m = mem(2 pages) >> At this point: >> PAGE-1 PAGE-2 >> VM_SOFTDIRTY set set >> PTE softdirty flag not set not set >> /proc//pagemap view set set >> memset(m) >> At this point: >> PAGE-1 PAGE-2 >> VM_SOFTDIRTY set set >> PTE softdirty flag set set >> /proc//pagemap view set set >> >> This example proves that soft-dirty flag on the PTE is set regardless of >> the VM_SOFTDIRTY. > > Hi Andrew and Cyrill, > > Peter doesn't agree with me here that this change in behavior should be > reverted etc. Please comment.
For the records, I agree with Peter: As 76aefad628aa ("mm/mprotect: fix soft-dirty check in can_change_pte_writable()") documents, this patch fixed real problems.
/proc/pagemap works as expected right now such that we don't have an under-indication. Internal representation is an implementation detail.
Whatever we do, there must not be an under-indication of softdirty. That is the ABI guaranteed (especially for anonymous memory). "No over-indication" was never the ABI guarantee.
For your use case, you want to reduce over-indication. I suggested looked into alternatives.
-- Thanks,
David / dhildenb
| |