On Thu, Nov 17, 2022 at 10:12:07AM -0500, Peter Xu wrote:
Hi, Huacai,
On Thu, Nov 17, 2022 at 12:25:32PM +0800, Huacai Chen wrote:
Now {pmd,pte}_mkdirty() set _PAGE_DIRTY bit unconditionally, this causes random segmentation fault after commit 0ccf7f168e17bb7e ("mm/thp: carry over dirty bit when thp splits on pmd").
The reason is: when fork(), parent process use pmd_wrprotect() to clear huge page's _PAGE_WRITE and _PAGE_DIRTY (for COW);
Is it safe to drop dirty bit when wr-protect? It means the mm can reclaim the page directly assuming the page contains rubbish.
Consider after fork() and memory pressure kicks the kswapd, I don't see anything stops the kswapd from recycling the pages and lose the data in both processes.
Feel free to ignore this question.. I think I got an answer from Hev (and I then got a follow up question):
https://lore.kernel.org/all/Y3Z9Zf0jARMOkFBq@x1n/
then pte_mkdirty() set _PAGE_DIRTY as well as _PAGE_MODIFIED while splitting dirty huge pages; once _PAGE_DIRTY is set, there will be no tlb modify exception so the COW machanism fails; and at last memory corruption occurred between parent and child processes.
So, we should set _PAGE_DIRTY only when _PAGE_WRITE is set in {pmd,pte}_ mkdirty().
Cc: stable@vger.kernel.org Cc: Peter Xu peterx@redhat.com Signed-off-by: Huacai Chen chenhuacai@loongson.cn
Note: CC sparc maillist because they have similar issues.
I also had a look on sparc64, it seems to not do the same as loongarch here (not removing dirty in wr-protect):
static inline pmd_t pmd_wrprotect(pmd_t pmd) { pte_t pte = __pte(pmd_val(pmd));
pte = pte_wrprotect(pte);
return __pmd(pte_val(pte)); }
static inline pte_t pte_wrprotect(pte_t pte) { unsigned long val = pte_val(pte), tmp;
__asm__ __volatile__( "\n661: andn %0, %3, %0\n" " nop\n" "\n662: nop\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" " andn %0, %1, %0\n" " .previous\n" : "=r" (val), "=r" (tmp) : "0" (val), "i" (_PAGE_WRITE_4U | _PAGE_W_4U), "i" (_PAGE_WRITE_4V | _PAGE_W_4V));
return __pte(val); }
(Same here; I just overlooked what does _PAGE_W_4U meant..)
arch/loongarch/include/asm/pgtable.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h index 946704bee599..debbe116f105 100644 --- a/arch/loongarch/include/asm/pgtable.h +++ b/arch/loongarch/include/asm/pgtable.h @@ -349,7 +349,9 @@ static inline pte_t pte_mkclean(pte_t pte) static inline pte_t pte_mkdirty(pte_t pte) {
- pte_val(pte) |= (_PAGE_DIRTY | _PAGE_MODIFIED);
- pte_val(pte) |= _PAGE_MODIFIED;
- if (pte_val(pte) & _PAGE_WRITE)
pte_val(pte) |= _PAGE_DIRTY;I'm not sure whether mm has rule to always set write bit then set dirty bit, need to be careful here because the outcome may differ when use:
pte_mkdirty(pte_mkwrite(pte)) (expected)
VS:
pte_mkwrite(pte_mkdirty(pte)) (dirty not set)
I had a feeling I miss some arch-specific details here on why loongarch needs such implementation, but I can't quickly tell.
After a closer look I think it's fine for loongarch as pte_mkwrite will also set the dirty bit unconditionally, so at least the two ways will still generate the same pte (DIRTY+MODIFIED+WRITE).
But this whole thing is still confusing to me. It'll still be great if anyone can help explain why the _DIRTY cannot be set only in pte_mkwrite() if that's the solo place in charge of "whether the pte is writable".
The other follow up question is: how do we mark "this pte contains valid data" (the common definition of "dirty bit"), while "this pte is not writable" on loongarch?
It can happen when we're installing a page with non-zero data meanwhile wr-protected. That's actually a valid case for userfaultfd wr-protect mode where user specified UFFDIO_COPY ioctl with flag UFFDIO_COPY_MODE_WP, where we'll install a non-zero page from user buffer but don't grant write bit.
From code-wise, I think it can be done currently with this on loongarch:
pte_wrprotect(pte_mkwrite(pte_mkdirty(pte)))
Where pte_wrprotect(pte_mkwrite(pte)) is not a no-op but applying MODIFIED.
While on many other archs it'll be as simple as:
pte_mkdirty(pte)
But that's really error-prone and not obvious.
Copying Hev too.
Thanks,