On Feb 17, 2022, at 8:00 PM, Nadav Amit nadav.amit@gmail.com wrote:
On Feb 17, 2022, at 6:23 PM, Nadav Amit nadav.amit@gmail.com wrote:
PS: I always think here the VM_SOFTDIRTY check is wrong, IMHO it should be:
if (dirty_accountable && pte_dirty(ptent) && (pte_soft_dirty(ptent) || (vma->vm_flags & VM_SOFTDIRTY))) { ptent = pte_mkwrite(ptent); }I know it is off-topic (not directly related to my patch), but I tried to understand the logic - both of the existing code and of your suggested change - and I failed.
IIUC dirty_accountable (whose value is taken from vma_wants_writenotify()) means that the writes *should* be tracked, and therefore the page should remain read-only.
So basically the condition should have been based on !dirty_accountable, i.e. the inverted value of dirty_accountable.
The problem is that dirty_accountable also reflects VM_SOFTDIRTY considerations, so looking on the PTE does not tell you whether the PTE should remain write-protected even if it is dirty.
Just as I pressed send, I understood your suggestion.
It is still confusing for me how setting write-permissions for dirty_accountable PTEs is safe (as long as they are dirty), and yet in the general case it is not safe. I need to further think of it.