* + mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable.patch added to mm-unstable branch
@ 2022-07-21 3:09 Andrew Morton
0 siblings, 0 replies; 2+ messages in thread
From: Andrew Morton @ 2022-07-21 3:09 UTC (permalink / raw)
To: mm-commits, peterx, akpm
The patch titled
Subject: mm/mprotect: fix soft-dirty check in can_change_pte_writable()
has been added to the -mm mm-unstable branch. Its filename is
mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable.patch
This patch will shortly appear at
https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable.patch
This patch will later appear in the mm-unstable branch at
git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a
reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
The -mm tree is included into linux-next via the mm-everything
branch at git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
and is updated there every 2-3 working days
------------------------------------------------------
From: Peter Xu <peterx@redhat.com>
Subject: mm/mprotect: fix soft-dirty check in can_change_pte_writable()
Date: Wed, 20 Jul 2022 18:03:24 -0400
The check wanted to make sure when soft-dirty tracking is enabled we won't
grant write bit by accident, as a page fault is needed for dirty tracking.
The intention is correct but we didn't check it right because
VM_SOFTDIRTY set actually means soft-dirty tracking disabled. Fix it.
It wasn't a bug for a long time because we used to only optimize the write
bit settings in change_pte_range() for page caches, and since we've got a
higher level check in vma_wants_writenotify(), we will never set the bit
MM_CP_TRY_CHANGE_WRITABLE for soft-dirty enabled page caches, hence even
if we checked with the wrong value of VM_SOFTDIRTY in change_pte_range()
it'll just be an no-op. Functionally it was still correct, even if cpu
cycles wasted.
However after the recent work of anonymous page optimization on exclusive
pages we'll start to make it wrong because anonymous page does not require
the check in vma_wants_writenotify() hence it'll suffer from the wrong
check here in can_change_pte_writable().
We can easily verify this with any exclusive anonymous page, like program
below:
=======8<======
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <assert.h>
#include <inttypes.h>
#include <stdint.h>
#include <sys/types.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdbool.h>
#define BIT_ULL(nr) (1ULL << (nr))
#define PM_SOFT_DIRTY BIT_ULL(55)
unsigned int psize;
char *page;
uint64_t pagemap_read_vaddr(int fd, void *vaddr)
{
uint64_t value;
int ret;
ret = pread(fd, &value, sizeof(uint64_t),
((uint64_t)vaddr >> 12) * sizeof(uint64_t));
assert(ret == sizeof(uint64_t));
return value;
}
void clear_refs_write(void)
{
int fd = open("/proc/self/clear_refs", O_RDWR);
assert(fd >= 0);
write(fd, "4", 2);
close(fd);
}
#define check_soft_dirty(str, expect) do { \
bool dirty = pagemap_read_vaddr(fd, page) & PM_SOFT_DIRTY; \
if (dirty != expect) { \
printf("ERROR: %s, soft-dirty=%d (expect: %d)\n", str, dirty, expect); \
exit(-1); \
} \
} while (0)
int main(void)
{
int fd = open("/proc/self/pagemap", O_RDONLY);
assert(fd >= 0);
psize = getpagesize();
page = mmap(NULL, psize, PROT_READ|PROT_WRITE,
MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
assert(page != MAP_FAILED);
*page = 1;
check_soft_dirty("Just faulted in page", 1);
clear_refs_write();
check_soft_dirty("Clear_refs written", 0);
mprotect(page, psize, PROT_READ);
check_soft_dirty("Marked RO", 0);
mprotect(page, psize, PROT_READ|PROT_WRITE);
check_soft_dirty("Marked RW", 0);
*page = 2;
check_soft_dirty("Wrote page again", 1);
munmap(page, psize);
close(fd);
printf("Test passed.\n");
return 0;
}
=======8<======
So even if commit 64fe24a3e05e kept the old behavior and didn't attempt to
change the behavior here, the bug will only be able to be triggered after
commit 64fe24a3e05e because only anonymous page will suffer from it.
Link: https://lkml.kernel.org/r/20220720220324.88538-1-peterx@redhat.com
Fixes: 64fe24a3e05e ("mm/mprotect: try avoiding write faults for exclusive anonymous pages when changing protection")
Signed-off-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
mm/mprotect.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
--- a/mm/mprotect.c~mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable
+++ a/mm/mprotect.c
@@ -48,8 +48,11 @@ static inline bool can_change_pte_writab
if (pte_protnone(pte) || !pte_dirty(pte))
return false;
- /* Do we need write faults for softdirty tracking? */
- if ((vma->vm_flags & VM_SOFTDIRTY) && !pte_soft_dirty(pte))
+ /*
+ * Do we need write faults for softdirty tracking? Note,
+ * soft-dirty is enabled when !VM_SOFTDIRTY.
+ */
+ if (!(vma->vm_flags & VM_SOFTDIRTY) && !pte_soft_dirty(pte))
return false;
/* Do we need write faults for uffd-wp tracking? */
_
Patches currently in -mm which might be from peterx@redhat.com are
mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable.patch
^ permalink raw reply [flat|nested] 2+ messages in thread
* + mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable.patch added to mm-unstable branch
@ 2022-07-25 22:10 Andrew Morton
0 siblings, 0 replies; 2+ messages in thread
From: Andrew Morton @ 2022-07-25 22:10 UTC (permalink / raw)
To: mm-commits, nadav.amit, david, aarcange, peterx, akpm
The patch titled
Subject: mm/mprotect: fix soft-dirty check in can_change_pte_writable()
has been added to the -mm mm-unstable branch. Its filename is
mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable.patch
This patch will shortly appear at
https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable.patch
This patch will later appear in the mm-unstable branch at
git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a
reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
The -mm tree is included into linux-next via the mm-everything
branch at git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
and is updated there every 2-3 working days
------------------------------------------------------
From: Peter Xu <peterx@redhat.com>
Subject: mm/mprotect: fix soft-dirty check in can_change_pte_writable()
Date: Mon, 25 Jul 2022 10:20:46 -0400
Patch series "mm/mprotect: Fix soft-dirty checks", v4.
This patch (of 3):
The check wanted to make sure when soft-dirty tracking is enabled we won't
grant write bit by accident, as a page fault is needed for dirty tracking.
The intention is correct but we didn't check it right because
VM_SOFTDIRTY set actually means soft-dirty tracking disabled. Fix it.
There's another thing tricky about soft-dirty is that, we can't check the
vma flag !(vma_flags & VM_SOFTDIRTY) directly but only check it after we
checked CONFIG_MEM_SOFT_DIRTY because otherwise VM_SOFTDIRTY will be
defined as zero, and !(vma_flags & VM_SOFTDIRTY) will constantly return
true. To avoid misuse, introduce a helper for checking whether vma has
soft-dirty tracking enabled.
We can easily verify this with any exclusive anonymous page, like program
below:
=======8<======
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <assert.h>
#include <inttypes.h>
#include <stdint.h>
#include <sys/types.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdbool.h>
#define BIT_ULL(nr) (1ULL << (nr))
#define PM_SOFT_DIRTY BIT_ULL(55)
unsigned int psize;
char *page;
uint64_t pagemap_read_vaddr(int fd, void *vaddr)
{
uint64_t value;
int ret;
ret = pread(fd, &value, sizeof(uint64_t),
((uint64_t)vaddr >> 12) * sizeof(uint64_t));
assert(ret == sizeof(uint64_t));
return value;
}
void clear_refs_write(void)
{
int fd = open("/proc/self/clear_refs", O_RDWR);
assert(fd >= 0);
write(fd, "4", 2);
close(fd);
}
#define check_soft_dirty(str, expect) do { \
bool dirty = pagemap_read_vaddr(fd, page) & PM_SOFT_DIRTY; \
if (dirty != expect) { \
printf("ERROR: %s, soft-dirty=%d (expect: %d)
", str, dirty, expect); \
exit(-1); \
} \
} while (0)
int main(void)
{
int fd = open("/proc/self/pagemap", O_RDONLY);
assert(fd >= 0);
psize = getpagesize();
page = mmap(NULL, psize, PROT_READ|PROT_WRITE,
MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
assert(page != MAP_FAILED);
*page = 1;
check_soft_dirty("Just faulted in page", 1);
clear_refs_write();
check_soft_dirty("Clear_refs written", 0);
mprotect(page, psize, PROT_READ);
check_soft_dirty("Marked RO", 0);
mprotect(page, psize, PROT_READ|PROT_WRITE);
check_soft_dirty("Marked RW", 0);
*page = 2;
check_soft_dirty("Wrote page again", 1);
munmap(page, psize);
close(fd);
printf("Test passed.
");
return 0;
}
=======8<======
Here we attach a Fixes to commit 64fe24a3e05e only for easy tracking, as
this patch won't apply to a tree before that point. However the commit
wasn't the source of problem, but instead 64e455079e1b. It's just that
after 64fe24a3e05e anonymous memory will also suffer from this problem
with mprotect().
Link: https://lkml.kernel.org/r/20220725142048.30450-1-peterx@redhat.com
Link: https://lkml.kernel.org/r/20220725142048.30450-2-peterx@redhat.com
Fixes: 64e455079e1b ("mm: softdirty: enable write notifications on VMAs after VM_SOFTDIRTY cleared")
Fixes: 64fe24a3e05e ("mm/mprotect: try avoiding write faults for exclusive anonymous pages when changing protection")
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Cc: Nadav Amit <nadav.amit@gmail.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
mm/internal.h | 18 ++++++++++++++++++
mm/mmap.c | 2 +-
mm/mprotect.c | 2 +-
3 files changed, 20 insertions(+), 2 deletions(-)
--- a/mm/internal.h~mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable
+++ a/mm/internal.h
@@ -860,4 +860,22 @@ struct folio *try_grab_folio(struct page
DECLARE_PER_CPU(struct per_cpu_nodestat, boot_nodestats);
+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);
+}
+
#endif /* __MM_INTERNAL_H */
--- a/mm/mmap.c~mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable
+++ a/mm/mmap.c
@@ -1518,7 +1518,7 @@ int vma_wants_writenotify(struct vm_area
return 0;
/* Do we need to track softdirty? */
- if (IS_ENABLED(CONFIG_MEM_SOFT_DIRTY) && !(vm_flags & VM_SOFTDIRTY))
+ if (vma_soft_dirty_enabled(vma))
return 1;
/* Specialty mapping? */
--- a/mm/mprotect.c~mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable
+++ a/mm/mprotect.c
@@ -49,7 +49,7 @@ static inline bool can_change_pte_writab
return false;
/* Do we need write faults for softdirty tracking? */
- if ((vma->vm_flags & VM_SOFTDIRTY) && !pte_soft_dirty(pte))
+ if (vma_soft_dirty_enabled(vma) && !pte_soft_dirty(pte))
return false;
/* Do we need write faults for uffd-wp tracking? */
_
Patches currently in -mm which might be from peterx@redhat.com are
mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable.patch
selftests-soft-dirty-add-test-for-mprotect.patch
selftests-add-soft-dirty-into-run_vmtestssh.patch
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-07-25 22:11 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-25 22:10 + mm-mprotect-fix-soft-dirty-check-in-can_change_pte_writable.patch added to mm-unstable branch Andrew Morton
-- strict thread matches above, loose matches on Subject: below --
2022-07-21 3:09 Andrew Morton
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.