lkml.org 
[lkml]   [2022]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v3 1/3] mm/mprotect: Fix soft-dirty check in can_change_pte_writable()
On Fri, Jul 22, 2022 at 10:21:00AM -0700, Nadav Amit wrote:
> On Jul 22, 2022, at 12:08 AM, David Hildenbrand <david@redhat.com> wrote:
>
> >> +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma)
> >> +{
> >> + /*
> >> + * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty
> >> + * enablements, because when without soft-dirty being compiled in,
> >> + * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY)
> >> + * will be constantly true.
> >> + */
> >> + if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
> >> + return false;
> >> +
> >> + /*
> >> + * Soft-dirty is kind of special: its tracking is enabled when the
> >> + * vma flags not set.
> >> + */
> >> + return !(vma->vm_flags & VM_SOFTDIRTY);
> >> +}
> >
> > That will come in handy in other patches I'm cooking.
>
> clear_refs_write() also comes to mind as well (for consistency; I see no
> correctness or performance issue).

I explicitly didn't touch that because current code is better..

mas_for_each(&mas, vma, ULONG_MAX) {
if (!(vma->vm_flags & VM_SOFTDIRTY))
continue;
vma->vm_flags &= ~VM_SOFTDIRTY;
vma_set_page_prot(vma);
}

It means when !CONFIG_MEM_SOFT_DIRTY the "if" will always be true and all
vma will be jumped.

If replaced with vma_soft_dirty_enabled() it'll be instead constantly false
returned. We'll redo vma_set_page_prot() even if unnecessary.

Here if we want to add the "CONFIG_MEM_SOFT_DIRTY" into equation it can be:

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index f8cd58846a28..ab6f2913b5a5 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1290,6 +1290,8 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
}

if (type == CLEAR_REFS_SOFT_DIRTY) {
+ if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
+ goto out_unlock;
mas_for_each(&mas, vma, ULONG_MAX) {
if (!(vma->vm_flags & VM_SOFTDIRTY))
continue;
Or even at the entrance to not take the mm sem. But it's not anything
important IMHO, so if no one asking for that I'll just leave it be.

Thanks,

--
Peter Xu

\
 
 \ /
  Last update: 2022-07-25 16:00    [W:0.078 / U:0.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site