lkml.org 
[lkml]   [2022]   [Nov]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 04/47] LoongArch: Set _PAGE_DIRTY only if _PAGE_WRITE is set in {pmd,pte}_mkdirty()
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.

> 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);
}

>
> 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.

Thanks,

> return pte;
> }
>
> @@ -478,7 +480,9 @@ static inline pmd_t pmd_mkclean(pmd_t pmd)
>
> static inline pmd_t pmd_mkdirty(pmd_t pmd)
> {
> - pmd_val(pmd) |= (_PAGE_DIRTY | _PAGE_MODIFIED);
> + pmd_val(pmd) |= _PAGE_MODIFIED;
> + if (pmd_val(pmd) & _PAGE_WRITE)
> + pmd_val(pmd) |= _PAGE_DIRTY;
> return pmd;
> }
>
> --
> 2.31.1
>

--
Peter Xu

\
 
 \ /
  Last update: 2022-11-17 16:20    [W:1.260 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site