This is the follow-up on [1], adding selftests (testing for known issues
we added workarounds for and other issues that haven't been fixed yet),
fixing sparc64, reverting the workarounds, and perform one cleanup.
The patch from [1] was modified slightly (updated/extended patch
description, dropped one unnecessary NOP instruction from the ASM in
__pte_mkhwwrite()).
Retested on x86_64 and sparc64 (sun4u in QEMU).
I scanned most architectures to make sure their (pte|pmd)_mkdirty()
handling is correct. To be sure, we can run the selftests and find out if
other architectures are still affectes (loongarch was fixed recently as
well).
Based on master for now. I don't expect surprises regarding mm-tress, but
I can rebase if there are any problems.
[1] https://lkml.kernel.org/r/[email protected]
Cc: Andrew Morton <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Peter Xu <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Shuah Khan <[email protected]>
Cc: Sam Ravnborg <[email protected]>
Cc: Yu Zhao <[email protected]>
Cc: Anshuman Khandual <[email protected]>
David Hildenbrand (6):
selftests/mm: reuse read_pmd_pagesize() in COW selftest
selftests/mm: mkdirty: test behavior of (pte|pmd)_mkdirty on VMAs
without write permissions
sparc/mm: don't unconditionally set HW writable bit when setting PTE
dirty on 64bit
mm/migrate: revert "mm/migrate: fix wrongly apply write bit after
mkdirty on sparc64"
mm/huge_memory: revert "Partly revert "mm/thp: carry over dirty bit
when thp splits on pmd""
mm/huge_memory: conditionally call maybe_mkwrite() and drop
pte_wrprotect() in __split_huge_pmd_locked()
arch/sparc/include/asm/pgtable_64.h | 116 +++---
mm/huge_memory.c | 16 +-
mm/migrate.c | 2 -
tools/testing/selftests/mm/Makefile | 2 +
tools/testing/selftests/mm/cow.c | 33 +-
tools/testing/selftests/mm/khugepaged.c | 4 +
tools/testing/selftests/mm/mkdirty.c | 379 ++++++++++++++++++
tools/testing/selftests/mm/soft-dirty.c | 3 +
.../selftests/mm/split_huge_page_test.c | 4 +
tools/testing/selftests/mm/vm_util.c | 4 +-
10 files changed, 468 insertions(+), 95 deletions(-)
create mode 100644 tools/testing/selftests/mm/mkdirty.c
--
2.39.2
This reverts commit 96a9c287e25d ("mm/migrate: fix wrongly apply write bit
after mkdirty on sparc64").
Now that sparc64 mkdirty handling is fixed and no longer sets a PTE/PMD
writable that shouldn't be writable, let's revert the temporary fix.
The mkdirty mm selftest still passes with this change on sparc64.
Note that loongarch handling was fixed in commit bf2f34a506e6 ("LoongArch:
Set _PAGE_DIRTY only if _PAGE_WRITE is set in {pmd,pte}_mkdirty()").
Signed-off-by: David Hildenbrand <[email protected]>
---
mm/huge_memory.c | 6 ++----
mm/migrate.c | 2 --
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 032fb0ef9cd1..ec86bf1d4e81 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3276,6 +3276,8 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
pmde = mk_huge_pmd(new, READ_ONCE(vma->vm_page_prot));
if (pmd_swp_soft_dirty(*pvmw->pmd))
pmde = pmd_mksoft_dirty(pmde);
+ if (is_writable_migration_entry(entry))
+ pmde = maybe_pmd_mkwrite(pmde, vma);
if (pmd_swp_uffd_wp(*pvmw->pmd))
pmde = pmd_mkuffd_wp(pmde);
if (!is_migration_entry_young(entry))
@@ -3283,10 +3285,6 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
/* NOTE: this may contain setting soft-dirty on some archs */
if (PageDirty(new) && is_migration_entry_dirty(entry))
pmde = pmd_mkdirty(pmde);
- if (is_writable_migration_entry(entry))
- pmde = maybe_pmd_mkwrite(pmde, vma);
- else
- pmde = pmd_wrprotect(pmde);
if (PageAnon(new)) {
rmap_t rmap_flags = RMAP_COMPOUND;
diff --git a/mm/migrate.c b/mm/migrate.c
index db3f154446af..3b21c44e2176 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -225,8 +225,6 @@ static bool remove_migration_pte(struct folio *folio,
pte = maybe_mkwrite(pte, vma);
else if (pte_swp_uffd_wp(*pvmw.pte))
pte = pte_mkuffd_wp(pte);
- else
- pte = pte_wrprotect(pte);
if (folio_test_anon(folio) && !is_readable_migration_entry(entry))
rmap_flags |= RMAP_EXCLUSIVE;
--
2.39.2
This reverts commit 624a2c94f5b7 ("Partly revert "mm/thp: carry over dirty
bit when thp splits on pmd"") and the fixup in commit e833bc503405
("mm/thp: re-apply mkdirty for small pages after split").
Now that sparc64 mkdirty handling is fixed and no longer sets a PTE/PMD
writable that shouldn't be writable, let's revert the temporary fix and
remove the stale comment.
The mkdirty mm selftest still passes with this change on sparc64.
Note that loongarch handling was fixed in commit bf2f34a506e6 ("LoongArch:
Set _PAGE_DIRTY only if _PAGE_WRITE is set in {pmd,pte}_mkdirty()")
Signed-off-by: David Hildenbrand <[email protected]>
---
mm/huge_memory.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index ec86bf1d4e81..6f3af65435c8 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2238,18 +2238,13 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
entry = maybe_mkwrite(entry, vma);
if (anon_exclusive)
SetPageAnonExclusive(page + i);
+ if (!write)
+ entry = pte_wrprotect(entry);
if (!young)
entry = pte_mkold(entry);
/* NOTE: this may set soft-dirty too on some archs */
if (dirty)
entry = pte_mkdirty(entry);
- /*
- * NOTE: this needs to happen after pte_mkdirty,
- * because some archs (sparc64, loongarch) could
- * set hw write bit when mkdirty.
- */
- if (!write)
- entry = pte_wrprotect(entry);
if (soft_dirty)
entry = pte_mksoft_dirty(entry);
if (uffd_wp)
--
2.39.2
On sparc64, there is no HW modified bit, therefore, SW tracks via a SW
bit if the PTE is dirty via pte_mkdirty(). However, pte_mkdirty()
currently also unconditionally sets the HW writable bit, which is wrong.
pte_mkdirty() is not supposed to make a PTE actually writable, unless the
SW writable bit -- pte_write() -- indicates that the PTE is not
write-protected. Fortunately, sparc64 also defines a SW writable bit.
For example, this already turned into a problem in the context of
THP splitting as documented in commit 624a2c94f5b7 ("Partly revert "mm/thp:
carry over dirty bit when thp splits on pmd""), and for page migration,
as documented in commit 96a9c287e25d ("mm/migrate: fix wrongly apply write
bit after mkdirty on sparc64").
Also, we might want to use the dirty PTE bit in the context of KSM with
shared zeropage [1], whereby setting the page writable would be
problematic.
But more general, any code that might end up setting a PTE/PMD dirty
inside a VM without write permissions is possibly broken,
Before this commit (sun4u in QEMU):
root@debian:~/linux/tools/testing/selftests/mm# ./mkdirty
# [INFO] detected THP size: 8192 KiB
TAP version 13
1..6
# [INFO] PTRACE write access
not ok 1 SIGSEGV generated, page not modified
# [INFO] PTRACE write access to THP
not ok 2 SIGSEGV generated, page not modified
# [INFO] Page migration
ok 3 SIGSEGV generated, page not modified
# [INFO] Page migration of THP
ok 4 SIGSEGV generated, page not modified
# [INFO] PTE-mapping a THP
ok 5 SIGSEGV generated, page not modified
# [INFO] UFFDIO_COPY
not ok 6 SIGSEGV generated, page not modified
Bail out! 3 out of 6 tests failed
# Totals: pass:3 fail:3 xfail:0 xpass:0 skip:0 error:0
Test #3,#4,#5 pass ever since we added some MM workarounds, the
underlying issue remains.
Let's fix the remaining issues and prepare for reverting the workarounds
by setting the HW writable bit only if both, the SW dirty bit and the SW
writable bit are set.
We have to move pte_dirty() and pte_dirty() up. The code patching
mechanism and handling constants > 22bit is a bit special on sparc64.
The ASM logic in pte_mkdirty() and pte_mkwrite() match the logic in
pte_mkold() to create the mask depending on the machine type. The ASM
logic in __pte_mkhwwrite() matches the logic in pte_present(), just
using an "or" instead of an "and" instruction.
With this commit (sun4u in QEMU):
root@debian:~/linux/tools/testing/selftests/mm# ./mkdirty
# [INFO] detected THP size: 8192 KiB
TAP version 13
1..6
# [INFO] PTRACE write access
ok 1 SIGSEGV generated, page not modified
# [INFO] PTRACE write access to THP
ok 2 SIGSEGV generated, page not modified
# [INFO] Page migration
ok 3 SIGSEGV generated, page not modified
# [INFO] Page migration of THP
ok 4 SIGSEGV generated, page not modified
# [INFO] PTE-mapping a THP
ok 5 SIGSEGV generated, page not modified
# [INFO] UFFDIO_COPY
ok 6 SIGSEGV generated, page not modified
# Totals: pass:6 fail:0 xfail:0 xpass:0 skip:0 error:0
This handling seems to have been in place forever.
[1] https://lkml.kernel.org/r/[email protected]
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: David Hildenbrand <[email protected]>
---
arch/sparc/include/asm/pgtable_64.h | 116 ++++++++++++++++------------
1 file changed, 66 insertions(+), 50 deletions(-)
diff --git a/arch/sparc/include/asm/pgtable_64.h b/arch/sparc/include/asm/pgtable_64.h
index 2dc8d4641734..5563efa1a19f 100644
--- a/arch/sparc/include/asm/pgtable_64.h
+++ b/arch/sparc/include/asm/pgtable_64.h
@@ -357,6 +357,42 @@ static inline pgprot_t pgprot_noncached(pgprot_t prot)
*/
#define pgprot_noncached pgprot_noncached
+static inline unsigned long pte_dirty(pte_t pte)
+{
+ unsigned long mask;
+
+ __asm__ __volatile__(
+ "\n661: mov %1, %0\n"
+ " nop\n"
+ " .section .sun4v_2insn_patch, \"ax\"\n"
+ " .word 661b\n"
+ " sethi %%uhi(%2), %0\n"
+ " sllx %0, 32, %0\n"
+ " .previous\n"
+ : "=r" (mask)
+ : "i" (_PAGE_MODIFIED_4U), "i" (_PAGE_MODIFIED_4V));
+
+ return (pte_val(pte) & mask);
+}
+
+static inline unsigned long pte_write(pte_t pte)
+{
+ unsigned long mask;
+
+ __asm__ __volatile__(
+ "\n661: mov %1, %0\n"
+ " nop\n"
+ " .section .sun4v_2insn_patch, \"ax\"\n"
+ " .word 661b\n"
+ " sethi %%uhi(%2), %0\n"
+ " sllx %0, 32, %0\n"
+ " .previous\n"
+ : "=r" (mask)
+ : "i" (_PAGE_WRITE_4U), "i" (_PAGE_WRITE_4V));
+
+ return (pte_val(pte) & mask);
+}
+
#if defined(CONFIG_HUGETLB_PAGE) || defined(CONFIG_TRANSPARENT_HUGEPAGE)
pte_t arch_make_huge_pte(pte_t entry, unsigned int shift, vm_flags_t flags);
#define arch_make_huge_pte arch_make_huge_pte
@@ -418,28 +454,43 @@ static inline bool is_hugetlb_pte(pte_t pte)
}
#endif
+static inline pte_t __pte_mkhwwrite(pte_t pte)
+{
+ unsigned long val = pte_val(pte);
+
+ /*
+ * Note: we only want to set the HW writable bit if the SW writable bit
+ * and the SW dirty bit are set.
+ */
+ __asm__ __volatile__(
+ "\n661: or %0, %2, %0\n"
+ " .section .sun4v_1insn_patch, \"ax\"\n"
+ " .word 661b\n"
+ " or %0, %3, %0\n"
+ " .previous\n"
+ : "=r" (val)
+ : "0" (val), "i" (_PAGE_W_4U), "i" (_PAGE_W_4V));
+
+ return __pte(val);
+}
+
static inline pte_t pte_mkdirty(pte_t pte)
{
- unsigned long val = pte_val(pte), tmp;
+ unsigned long val = pte_val(pte), mask;
__asm__ __volatile__(
- "\n661: or %0, %3, %0\n"
- " nop\n"
- "\n662: nop\n"
+ "\n661: mov %1, %0\n"
" nop\n"
" .section .sun4v_2insn_patch, \"ax\"\n"
" .word 661b\n"
- " sethi %%uhi(%4), %1\n"
- " sllx %1, 32, %1\n"
- " .word 662b\n"
- " or %1, %%lo(%4), %1\n"
- " or %0, %1, %0\n"
+ " sethi %%uhi(%2), %0\n"
+ " sllx %0, 32, %0\n"
" .previous\n"
- : "=r" (val), "=r" (tmp)
- : "0" (val), "i" (_PAGE_MODIFIED_4U | _PAGE_W_4U),
- "i" (_PAGE_MODIFIED_4V | _PAGE_W_4V));
+ : "=r" (mask)
+ : "i" (_PAGE_MODIFIED_4U), "i" (_PAGE_MODIFIED_4V));
- return __pte(val);
+ pte = __pte(val | mask);
+ return pte_write(pte) ? __pte_mkhwwrite(pte) : pte;
}
static inline pte_t pte_mkclean(pte_t pte)
@@ -481,7 +532,8 @@ static inline pte_t pte_mkwrite(pte_t pte)
: "=r" (mask)
: "i" (_PAGE_WRITE_4U), "i" (_PAGE_WRITE_4V));
- return __pte(val | mask);
+ pte = __pte(val | mask);
+ return pte_dirty(pte) ? __pte_mkhwwrite(pte) : pte;
}
static inline pte_t pte_wrprotect(pte_t pte)
@@ -584,42 +636,6 @@ static inline unsigned long pte_young(pte_t pte)
return (pte_val(pte) & mask);
}
-static inline unsigned long pte_dirty(pte_t pte)
-{
- unsigned long mask;
-
- __asm__ __volatile__(
- "\n661: mov %1, %0\n"
- " nop\n"
- " .section .sun4v_2insn_patch, \"ax\"\n"
- " .word 661b\n"
- " sethi %%uhi(%2), %0\n"
- " sllx %0, 32, %0\n"
- " .previous\n"
- : "=r" (mask)
- : "i" (_PAGE_MODIFIED_4U), "i" (_PAGE_MODIFIED_4V));
-
- return (pte_val(pte) & mask);
-}
-
-static inline unsigned long pte_write(pte_t pte)
-{
- unsigned long mask;
-
- __asm__ __volatile__(
- "\n661: mov %1, %0\n"
- " nop\n"
- " .section .sun4v_2insn_patch, \"ax\"\n"
- " .word 661b\n"
- " sethi %%uhi(%2), %0\n"
- " sllx %0, 32, %0\n"
- " .previous\n"
- : "=r" (mask)
- : "i" (_PAGE_WRITE_4U), "i" (_PAGE_WRITE_4V));
-
- return (pte_val(pte) & mask);
-}
-
static inline unsigned long pte_exec(pte_t pte)
{
unsigned long mask;
--
2.39.2
The COW selftest can deal with THP not being configured. So move error
handling of read_pmd_pagesize() into the callers such that we can reuse
it in the COW selftest.
Signed-off-by: David Hildenbrand <[email protected]>
---
tools/testing/selftests/mm/cow.c | 33 +++----------------
tools/testing/selftests/mm/khugepaged.c | 4 +++
tools/testing/selftests/mm/soft-dirty.c | 3 ++
.../selftests/mm/split_huge_page_test.c | 4 +++
tools/testing/selftests/mm/vm_util.c | 4 +--
5 files changed, 17 insertions(+), 31 deletions(-)
diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c
index 0eb2e8180aa5..dc9d6fe86028 100644
--- a/tools/testing/selftests/mm/cow.c
+++ b/tools/testing/selftests/mm/cow.c
@@ -45,34 +45,6 @@ static size_t hugetlbsizes[10];
static int gup_fd;
static bool has_huge_zeropage;
-static void detect_thpsize(void)
-{
- int fd = open("/sys/kernel/mm/transparent_hugepage/hpage_pmd_size",
- O_RDONLY);
- size_t size = 0;
- char buf[15];
- int ret;
-
- if (fd < 0)
- return;
-
- ret = pread(fd, buf, sizeof(buf), 0);
- if (ret > 0 && ret < sizeof(buf)) {
- buf[ret] = 0;
-
- size = strtoul(buf, NULL, 10);
- if (size < pagesize)
- size = 0;
- if (size > 0) {
- thpsize = size;
- ksft_print_msg("[INFO] detected THP size: %zu KiB\n",
- thpsize / 1024);
- }
- }
-
- close(fd);
-}
-
static void detect_huge_zeropage(void)
{
int fd = open("/sys/kernel/mm/transparent_hugepage/use_zero_page",
@@ -1741,7 +1713,10 @@ int main(int argc, char **argv)
int err;
pagesize = getpagesize();
- detect_thpsize();
+ thpsize = read_pmd_pagesize();
+ if (thpsize)
+ ksft_print_msg("[INFO] detected THP size: %zu KiB\n",
+ thpsize / 1024);
detect_hugetlbsizes();
detect_huge_zeropage();
diff --git a/tools/testing/selftests/mm/khugepaged.c b/tools/testing/selftests/mm/khugepaged.c
index 64126c8cd561..97adc0f34f9c 100644
--- a/tools/testing/selftests/mm/khugepaged.c
+++ b/tools/testing/selftests/mm/khugepaged.c
@@ -1476,6 +1476,10 @@ int main(int argc, const char **argv)
page_size = getpagesize();
hpage_pmd_size = read_pmd_pagesize();
+ if (!hpage_pmd_size) {
+ printf("Reading PMD pagesize failed");
+ exit(EXIT_FAILURE);
+ }
hpage_pmd_nr = hpage_pmd_size / page_size;
default_settings.khugepaged.max_ptes_none = hpage_pmd_nr - 1;
diff --git a/tools/testing/selftests/mm/soft-dirty.c b/tools/testing/selftests/mm/soft-dirty.c
index 21d8830c5f24..cc5f144430d4 100644
--- a/tools/testing/selftests/mm/soft-dirty.c
+++ b/tools/testing/selftests/mm/soft-dirty.c
@@ -80,6 +80,9 @@ static void test_hugepage(int pagemap_fd, int pagesize)
int i, ret;
size_t hpage_len = read_pmd_pagesize();
+ if (!hpage_len)
+ ksft_exit_fail_msg("Reading PMD pagesize failed");
+
map = memalign(hpage_len, hpage_len);
if (!map)
ksft_exit_fail_msg("memalign failed\n");
diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
index 76e1c36dd9e5..1dc5804b8b2b 100644
--- a/tools/testing/selftests/mm/split_huge_page_test.c
+++ b/tools/testing/selftests/mm/split_huge_page_test.c
@@ -300,6 +300,10 @@ int main(int argc, char **argv)
pagesize = getpagesize();
pageshift = ffs(pagesize) - 1;
pmd_pagesize = read_pmd_pagesize();
+ if (!pmd_pagesize) {
+ printf("Reading PMD pagesize failed\n");
+ exit(EXIT_FAILURE);
+ }
split_pmd_thp();
split_pte_mapped_thp();
diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
index 40e795624ff3..8dc74dd022c2 100644
--- a/tools/testing/selftests/mm/vm_util.c
+++ b/tools/testing/selftests/mm/vm_util.c
@@ -84,12 +84,12 @@ uint64_t read_pmd_pagesize(void)
fd = open(PMD_SIZE_FILE_PATH, O_RDONLY);
if (fd == -1)
- ksft_exit_fail_msg("Open hpage_pmd_size failed\n");
+ return 0;
num_read = read(fd, buf, 19);
if (num_read < 1) {
close(fd);
- ksft_exit_fail_msg("Read hpage_pmd_size failed\n");
+ return 0;
}
buf[num_read] = '\0';
close(fd);
--
2.39.2
No need to call maybe_mkwrite() to then wrprotect if the source PMD was not
writable.
It's worth nothing that this now allows for PTEs to be writable even if
the source PMD was not writable: if vma->vm_page_prot includes write
permissions.
As documented in commit 931298e103c2 ("mm/userfaultfd: rely on
vma->vm_page_prot in uffd_wp_range()"), any mechanism that intends to
have pages wrprotected (COW, writenotify, mprotect, uffd-wp, softdirty,
...) has to properly adjust vma->vm_page_prot upfront, to not include
write permissions. If vma->vm_page_prot includes write permissions, the
PTE/PMD can be writable as default.
This now mimics the handling in mm/migrate.c:remove_migration_pte() and in
mm/huge_memory.c:remove_migration_pmd(), which has been in place for a
long time (except that 96a9c287e25d ("mm/migrate: fix wrongly apply write
bit after mkdirty on sparc64") temporarily changed it).
Signed-off-by: David Hildenbrand <[email protected]>
---
mm/huge_memory.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 6f3af65435c8..8332e16ac97b 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2235,11 +2235,10 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
entry = pte_swp_mkuffd_wp(entry);
} else {
entry = mk_pte(page + i, READ_ONCE(vma->vm_page_prot));
- entry = maybe_mkwrite(entry, vma);
+ if (write)
+ entry = maybe_mkwrite(entry, vma);
if (anon_exclusive)
SetPageAnonExclusive(page + i);
- if (!write)
- entry = pte_wrprotect(entry);
if (!young)
entry = pte_mkold(entry);
/* NOTE: this may set soft-dirty too on some archs */
--
2.39.2
Hi David.
On Tue, Apr 11, 2023 at 04:25:09PM +0200, David Hildenbrand wrote:
> On sparc64, there is no HW modified bit, therefore, SW tracks via a SW
> bit if the PTE is dirty via pte_mkdirty(). However, pte_mkdirty()
> currently also unconditionally sets the HW writable bit, which is wrong.
>
> pte_mkdirty() is not supposed to make a PTE actually writable, unless the
> SW writable bit -- pte_write() -- indicates that the PTE is not
> write-protected. Fortunately, sparc64 also defines a SW writable bit.
>
> For example, this already turned into a problem in the context of
> THP splitting as documented in commit 624a2c94f5b7 ("Partly revert "mm/thp:
> carry over dirty bit when thp splits on pmd""), and for page migration,
> as documented in commit 96a9c287e25d ("mm/migrate: fix wrongly apply write
> bit after mkdirty on sparc64").
>
> Also, we might want to use the dirty PTE bit in the context of KSM with
> shared zeropage [1], whereby setting the page writable would be
> problematic.
>
> But more general, any code that might end up setting a PTE/PMD dirty
> inside a VM without write permissions is possibly broken,
>
> Before this commit (sun4u in QEMU):
> root@debian:~/linux/tools/testing/selftests/mm# ./mkdirty
> # [INFO] detected THP size: 8192 KiB
> TAP version 13
> 1..6
> # [INFO] PTRACE write access
> not ok 1 SIGSEGV generated, page not modified
> # [INFO] PTRACE write access to THP
> not ok 2 SIGSEGV generated, page not modified
> # [INFO] Page migration
> ok 3 SIGSEGV generated, page not modified
> # [INFO] Page migration of THP
> ok 4 SIGSEGV generated, page not modified
> # [INFO] PTE-mapping a THP
> ok 5 SIGSEGV generated, page not modified
> # [INFO] UFFDIO_COPY
> not ok 6 SIGSEGV generated, page not modified
> Bail out! 3 out of 6 tests failed
> # Totals: pass:3 fail:3 xfail:0 xpass:0 skip:0 error:0
>
> Test #3,#4,#5 pass ever since we added some MM workarounds, the
> underlying issue remains.
>
> Let's fix the remaining issues and prepare for reverting the workarounds
> by setting the HW writable bit only if both, the SW dirty bit and the SW
> writable bit are set.
>
> We have to move pte_dirty() and pte_dirty() up. The code patching
One of the pte_dirty() should be replaced with pte_write().
It would have been nice to separate moving and changes in two patches,
but keeping it together works too.
> mechanism and handling constants > 22bit is a bit special on sparc64.
>
> The ASM logic in pte_mkdirty() and pte_mkwrite() match the logic in
> pte_mkold() to create the mask depending on the machine type. The ASM
> logic in __pte_mkhwwrite() matches the logic in pte_present(), just
> using an "or" instead of an "and" instruction.
>
> With this commit (sun4u in QEMU):
> root@debian:~/linux/tools/testing/selftests/mm# ./mkdirty
> # [INFO] detected THP size: 8192 KiB
> TAP version 13
> 1..6
> # [INFO] PTRACE write access
> ok 1 SIGSEGV generated, page not modified
> # [INFO] PTRACE write access to THP
> ok 2 SIGSEGV generated, page not modified
> # [INFO] Page migration
> ok 3 SIGSEGV generated, page not modified
> # [INFO] Page migration of THP
> ok 4 SIGSEGV generated, page not modified
> # [INFO] PTE-mapping a THP
> ok 5 SIGSEGV generated, page not modified
> # [INFO] UFFDIO_COPY
> ok 6 SIGSEGV generated, page not modified
> # Totals: pass:6 fail:0 xfail:0 xpass:0 skip:0 error:0
Nice!
>
> This handling seems to have been in place forever.
>
> [1] https://lkml.kernel.org/r/[email protected]
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: David Hildenbrand <[email protected]>
I tried to follow your changes, but my knowledge of gcc assembler failed
me. But based on the nice and detailed change log and the code I managed
to understand:
Acked-by: Sam Ravnborg <[email protected]>
> ---
> arch/sparc/include/asm/pgtable_64.h | 116 ++++++++++++++++------------
> 1 file changed, 66 insertions(+), 50 deletions(-)
>
> diff --git a/arch/sparc/include/asm/pgtable_64.h b/arch/sparc/include/asm/pgtable_64.h
> index 2dc8d4641734..5563efa1a19f 100644
> --- a/arch/sparc/include/asm/pgtable_64.h
> +++ b/arch/sparc/include/asm/pgtable_64.h
> @@ -357,6 +357,42 @@ static inline pgprot_t pgprot_noncached(pgprot_t prot)
> */
> #define pgprot_noncached pgprot_noncached
>
> +static inline unsigned long pte_dirty(pte_t pte)
> +{
> + unsigned long mask;
> +
> + __asm__ __volatile__(
> + "\n661: mov %1, %0\n"
> + " nop\n"
> + " .section .sun4v_2insn_patch, \"ax\"\n"
> + " .word 661b\n"
> + " sethi %%uhi(%2), %0\n"
> + " sllx %0, 32, %0\n"
> + " .previous\n"
> + : "=r" (mask)
> + : "i" (_PAGE_MODIFIED_4U), "i" (_PAGE_MODIFIED_4V));
> +
> + return (pte_val(pte) & mask);
> +}
> +
> +static inline unsigned long pte_write(pte_t pte)
> +{
> + unsigned long mask;
> +
> + __asm__ __volatile__(
> + "\n661: mov %1, %0\n"
> + " nop\n"
> + " .section .sun4v_2insn_patch, \"ax\"\n"
> + " .word 661b\n"
> + " sethi %%uhi(%2), %0\n"
> + " sllx %0, 32, %0\n"
> + " .previous\n"
> + : "=r" (mask)
> + : "i" (_PAGE_WRITE_4U), "i" (_PAGE_WRITE_4V));
> +
> + return (pte_val(pte) & mask);
> +}
> +
> #if defined(CONFIG_HUGETLB_PAGE) || defined(CONFIG_TRANSPARENT_HUGEPAGE)
> pte_t arch_make_huge_pte(pte_t entry, unsigned int shift, vm_flags_t flags);
> #define arch_make_huge_pte arch_make_huge_pte
> @@ -418,28 +454,43 @@ static inline bool is_hugetlb_pte(pte_t pte)
> }
> #endif
>
> +static inline pte_t __pte_mkhwwrite(pte_t pte)
> +{
> + unsigned long val = pte_val(pte);
> +
> + /*
> + * Note: we only want to set the HW writable bit if the SW writable bit
> + * and the SW dirty bit are set.
> + */
> + __asm__ __volatile__(
> + "\n661: or %0, %2, %0\n"
> + " .section .sun4v_1insn_patch, \"ax\"\n"
> + " .word 661b\n"
> + " or %0, %3, %0\n"
> + " .previous\n"
> + : "=r" (val)
> + : "0" (val), "i" (_PAGE_W_4U), "i" (_PAGE_W_4V));
> +
> + return __pte(val);
> +}
> +
> static inline pte_t pte_mkdirty(pte_t pte)
> {
> - unsigned long val = pte_val(pte), tmp;
> + unsigned long val = pte_val(pte), mask;
>
> __asm__ __volatile__(
> - "\n661: or %0, %3, %0\n"
> - " nop\n"
> - "\n662: nop\n"
> + "\n661: mov %1, %0\n"
> " nop\n"
> " .section .sun4v_2insn_patch, \"ax\"\n"
> " .word 661b\n"
> - " sethi %%uhi(%4), %1\n"
> - " sllx %1, 32, %1\n"
> - " .word 662b\n"
> - " or %1, %%lo(%4), %1\n"
> - " or %0, %1, %0\n"
> + " sethi %%uhi(%2), %0\n"
> + " sllx %0, 32, %0\n"
> " .previous\n"
> - : "=r" (val), "=r" (tmp)
> - : "0" (val), "i" (_PAGE_MODIFIED_4U | _PAGE_W_4U),
> - "i" (_PAGE_MODIFIED_4V | _PAGE_W_4V));
> + : "=r" (mask)
> + : "i" (_PAGE_MODIFIED_4U), "i" (_PAGE_MODIFIED_4V));
>
> - return __pte(val);
> + pte = __pte(val | mask);
> + return pte_write(pte) ? __pte_mkhwwrite(pte) : pte;
> }
>
> static inline pte_t pte_mkclean(pte_t pte)
> @@ -481,7 +532,8 @@ static inline pte_t pte_mkwrite(pte_t pte)
> : "=r" (mask)
> : "i" (_PAGE_WRITE_4U), "i" (_PAGE_WRITE_4V));
>
> - return __pte(val | mask);
> + pte = __pte(val | mask);
> + return pte_dirty(pte) ? __pte_mkhwwrite(pte) : pte;
> }
>
> static inline pte_t pte_wrprotect(pte_t pte)
> @@ -584,42 +636,6 @@ static inline unsigned long pte_young(pte_t pte)
> return (pte_val(pte) & mask);
> }
>
> -static inline unsigned long pte_dirty(pte_t pte)
> -{
> - unsigned long mask;
> -
> - __asm__ __volatile__(
> - "\n661: mov %1, %0\n"
> - " nop\n"
> - " .section .sun4v_2insn_patch, \"ax\"\n"
> - " .word 661b\n"
> - " sethi %%uhi(%2), %0\n"
> - " sllx %0, 32, %0\n"
> - " .previous\n"
> - : "=r" (mask)
> - : "i" (_PAGE_MODIFIED_4U), "i" (_PAGE_MODIFIED_4V));
> -
> - return (pte_val(pte) & mask);
> -}
> -
> -static inline unsigned long pte_write(pte_t pte)
> -{
> - unsigned long mask;
> -
> - __asm__ __volatile__(
> - "\n661: mov %1, %0\n"
> - " nop\n"
> - " .section .sun4v_2insn_patch, \"ax\"\n"
> - " .word 661b\n"
> - " sethi %%uhi(%2), %0\n"
> - " sllx %0, 32, %0\n"
> - " .previous\n"
> - : "=r" (mask)
> - : "i" (_PAGE_WRITE_4U), "i" (_PAGE_WRITE_4V));
> -
> - return (pte_val(pte) & mask);
> -}
> -
> static inline unsigned long pte_exec(pte_t pte)
> {
> unsigned long mask;
> --
> 2.39.2
[...]
>> Let's fix the remaining issues and prepare for reverting the workarounds
>> by setting the HW writable bit only if both, the SW dirty bit and the SW
>> writable bit are set.
>>
>> We have to move pte_dirty() and pte_dirty() up. The code patching
> One of the pte_dirty() should be replaced with pte_write().
>
Indeed, thanks. I assume Andrew can change the latter to pte_write().
[unless I have to resend, of course]
> It would have been nice to separate moving and changes in two patches,
> but keeping it together works too.
I prefer to not have "move code within the same file around" in separate
patches as long as it doesn't add too much noise. Here, I think it's
acceptable.
>
>
>> mechanism and handling constants > 22bit is a bit special on sparc64.
>>
>> The ASM logic in pte_mkdirty() and pte_mkwrite() match the logic in
>> pte_mkold() to create the mask depending on the machine type. The ASM
>> logic in __pte_mkhwwrite() matches the logic in pte_present(), just
>> using an "or" instead of an "and" instruction.
>>
>> With this commit (sun4u in QEMU):
>> root@debian:~/linux/tools/testing/selftests/mm# ./mkdirty
>> # [INFO] detected THP size: 8192 KiB
>> TAP version 13
>> 1..6
>> # [INFO] PTRACE write access
>> ok 1 SIGSEGV generated, page not modified
>> # [INFO] PTRACE write access to THP
>> ok 2 SIGSEGV generated, page not modified
>> # [INFO] Page migration
>> ok 3 SIGSEGV generated, page not modified
>> # [INFO] Page migration of THP
>> ok 4 SIGSEGV generated, page not modified
>> # [INFO] PTE-mapping a THP
>> ok 5 SIGSEGV generated, page not modified
>> # [INFO] UFFDIO_COPY
>> ok 6 SIGSEGV generated, page not modified
>> # Totals: pass:6 fail:0 xfail:0 xpass:0 skip:0 error:0
> Nice!
>
>>
>> This handling seems to have been in place forever.
>>
>> [1] https://lkml.kernel.org/r/[email protected]
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Signed-off-by: David Hildenbrand <[email protected]>
>
> I tried to follow your changes, but my knowledge of gcc assembler failed
> me. But based on the nice and detailed change log and the code I managed
> to understand:
> Acked-by: Sam Ravnborg <[email protected]>
Thanks Sam!
--
Thanks,
David / dhildenb
On Wed, 12 Apr 2023 11:48:15 +0200 David Hildenbrand <[email protected]> wrote:
> >> We have to move pte_dirty() and pte_dirty() up. The code patching
> > One of the pte_dirty() should be replaced with pte_write().
> >
>
> Indeed, thanks. I assume Andrew can change the latter to pte_write().
It was a struggle, but I managed to do this ;)