On Mon, Jun 28, 2021 at 7:40 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
>
> - /* Avoid taking write faults for known dirty pages */
> - if (dirty_accountable && pte_dirty(ptent) &&
> - (pte_soft_dirty(ptent) ||
> - !(vma->vm_flags & VM_SOFTDIRTY))) {
> + if (may_avoid_write_fault(ptent, vma, cp_flags))
> ptent = pte_mkwrite(ptent);
> - }
Hmm. I don't think this is correct.
As fat as I can tell, may_avoid_write_fault() doesn't even check if
the vma is writable!
Am I misreading it? Because I think you just made even a shared mmap
with "mprotect(PROT_READ)" turn the pte's writable.
Which is a "slight" security issue.
Maybe the new code is fine, and I'm missing something. The old code
looks strange too, which makes me think that the MM_CP_DIRTY_ACCT test
ends up saving us and depend on VM_WRITE. But it's very much not
obvious.
And even if I _am_ missing something, I really would like a very
obvious and direct test for "this vma is writable", ie maybe a
if (!(vma->vm_flags & VM_WRITE))
return false;
at the very top of the function.
And no, "pte_dirty()" is not a reason to make something writable, it
might have started out as a writable mapping, and we dirtied the page,
and we made it read-only. The page stays dirty, but it shouldn't
become writable just because of that.
So please make me get the warm and fuzzies about this code. Because
as-is, it just looks scary.
Linus
