Re: Test case for "mm/thp: carry over dirty bit when thp splits on pmd"

[Peter Xu <peterx@redhat.com>]

On Thu, Nov 17, 2022 at 10:29:57AM +0800, hev wrote:
> Hi Peter,

Hi, Hev,

> 
> On Thu, Nov 17, 2022 at 12:25 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Wed, Nov 16, 2022 at 01:45:15PM +0300, Anatoly Pugachev wrote:
> > > On Wed, Nov 16, 2022 at 11:49 AM hev <r@hev.cc> wrote:
> > > >
> > > > Hello Peter,
> >
> > Hi, Hev,
> >
> > Thanks for letting me know.
> >
> > > >
> > > > I see a random crash issue  on the LoongArch system, that is caused by
> > > > commit 0ccf7f1 ("mm/thp: carry over dirty bit when thp splits on
> > > > pmd").
> > > >
> > > > Now, the thing is already resolved. The root cause is arch's mkdirty
> > > > is set hardware writable bit in unconditional. That breaks
> > > > write-protect and then breaks COW.
> >
> > Could you help explain how that happened?
> >
> > I'm taking example of loongarch here:
> >
> > static inline pte_t pte_mkdirty(pte_t pte)
> > {
> >         pte_val(pte) |= (_PAGE_DIRTY | _PAGE_MODIFIED);
> >         return pte;
> > }
> >
> > #define _PAGE_MODIFIED          (_ULCAST_(1) << _PAGE_MODIFIED_SHIFT)
> > #define _PAGE_MODIFIED_SHIFT    9
> 
> _PAGE_MODIFIED is a software dirty bit
> 
> > #define _PAGE_DIRTY             (_ULCAST_(1) << _PAGE_DIRTY_SHIFT)
> > #define _PAGE_DIRTY_SHIFT       1
> 
> _PAGE_DIRTY is a hardware writable bit (bad naming), meaning that mmu
> allows write memory without any exception raised.

(I just missed this email before I reply to the other one, I should have
 read this one first..)

I see. This surprises me a bit, as I can't quickly tell how it'll always
work with the generic mm code.

Say, is there a quick answer on why _PAGE_DIRTY is set here rather than
pte_mkwrite()?  Because AFAIU that's where the mm wants to grant write
permission to a page table entry as the API, no?

> 
> >
> > I don't see when write bit is set, which is bit 8 instead:
> >
> > #define _PAGE_WRITE             (_ULCAST_(1) << _PAGE_WRITE_SHIFT)
> > #define _PAGE_WRITE_SHIFT       8
> 
> _PAGE_WRITE is a software writable bit (not hardware).
> 
> As David said, In __split_huge_pmd_locked, the VMA does not include VM_WRITE,
> 
> entry = maybe_mkwrite(entry, vma);
> 
> so the pte does not include software writable bit (_PAGE_WRITE).

Are you sure?  In your test case you mapped with RW, IIUC it means even
after the fork() VM_WRITE is set on both sides?

But I agree the write bit is not set, not because !VM_WRITE, but because we
take care of that explicitly to make sure pte has the same write bit as pmd:

                (pmd used to be wr-protected due to fork())
		write = pmd_write(old_pmd);
                ...

                (then when split pte shouldn't have write bit too)
                if (!write)
                        entry = pte_wrprotect(entry);

> 
> and the dirty is true,
> 
> if (dirty)
>     entry = pte_mkdirty(entry);
> 
> so the incorrect arch's pte_mkdirty set hardware writable
> bit(_PAGE_DIRTY) in unconditional for read-only pages.

True, that does also apply to sparc64 pte_mkdirty() with _PAGE_W_4[UV].  I
should have noticed earlier that its comment told me that's a write bit
already..

#define _PAGE_W_4U	  _AC(0x0000000000000002,UL) /* Writable             */

Thanks,

-- 
Peter Xu