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

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

[Anatoly Pugachev]

On Wed, Nov 16, 2022 at 11:49 AM hev <r@hev.cc> wrote:
>
> Hello Peter,
>
> 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.
>
> Here is a simple and fast testcase (It may be helpful for sparc64):
> https://gist.github.com/heiher/72919fae6b53f04cac606a9631100506
> (assertion: c sum == 0)

Just tried on my sparc64 VM -  fixed vs old (non-patched) kernels...

fixed kernel (6.1.0-rc5) running ./a.out:
mator@ttip:~$ ./a.out
c sum: 0
p sum: 35184372088832
c sum: 0
p sum: 35184372088832
c sum: 0
p sum: 35184372088832
c sum: 0
p sum: 35184372088832
c sum: 0
p sum: 35184372088832
...

old (non-patched) kernel (6.1.0-rc4) :
mator@ttip:~$ ./a.out
c sum: 35150012350464
p sum: 35184372088832
c sum: 35150012350464
p sum: 35184372088832
...

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

[David Hildenbrand]

On 16.11.22 11:45, Anatoly Pugachev wrote:
> On Wed, Nov 16, 2022 at 11:49 AM hev <r@hev.cc> wrote:
>>
>> Hello Peter,
>>
>> 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.

Ehm, does this imply that

pte = maybe_mkwrite(pte_mkdirty(pte), vma);

will result in a writable PTE, even though the VMA does not include 
VM_WRITE?

That would be really broken.

-- 
Thanks,

David / dhildenb

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

[Peter Xu]

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
#define _PAGE_DIRTY		(_ULCAST_(1) << _PAGE_DIRTY_SHIFT)
#define	_PAGE_DIRTY_SHIFT	1

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

According to loongarch spec:

https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#section-multi-level-page-table-structure-supported-by-page-walking

Bits 1 & 8 match the spec D & W definitions.  Bit 9 seems not defined but I
didn't quickly spot how that's related to the write bit.

> >
> > Here is a simple and fast testcase (It may be helpful for sparc64):
> > https://gist.github.com/heiher/72919fae6b53f04cac606a9631100506
> > (assertion: c sum == 0)
> 
> Just tried on my sparc64 VM -  fixed vs old (non-patched) kernels...
> 
> fixed kernel (6.1.0-rc5) running ./a.out:
> mator@ttip:~$ ./a.out
> c sum: 0
> p sum: 35184372088832
> c sum: 0
> p sum: 35184372088832
> c sum: 0
> p sum: 35184372088832
> c sum: 0
> p sum: 35184372088832
> c sum: 0
> p sum: 35184372088832
> ...
> 
> old (non-patched) kernel (6.1.0-rc4) :
> mator@ttip:~$ ./a.out
> c sum: 35150012350464
> p sum: 35184372088832
> c sum: 35150012350464
> p sum: 35184372088832
> ...

Thanks for the quick run, Anatoly.  Obviously I went the wrong way before
on the code patching.  It seems we have more chance fixing this.

-- 
Peter Xu

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

[hev]

Hi Peter,

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

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.

Regards,
Ray

>
> According to loongarch spec:
>
> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#section-multi-level-page-table-structure-supported-by-page-walking
>
> Bits 1 & 8 match the spec D & W definitions.  Bit 9 seems not defined but I
> didn't quickly spot how that's related to the write bit.
>
> > >
> > > Here is a simple and fast testcase (It may be helpful for sparc64):
> > > https://gist.github.com/heiher/72919fae6b53f04cac606a9631100506
> > > (assertion: c sum == 0)
> >
> > Just tried on my sparc64 VM -  fixed vs old (non-patched) kernels...
> >
> > fixed kernel (6.1.0-rc5) running ./a.out:
> > mator@ttip:~$ ./a.out
> > c sum: 0
> > p sum: 35184372088832
> > c sum: 0
> > p sum: 35184372088832
> > c sum: 0
> > p sum: 35184372088832
> > c sum: 0
> > p sum: 35184372088832
> > c sum: 0
> > p sum: 35184372088832
> > ...
> >
> > old (non-patched) kernel (6.1.0-rc4) :
> > mator@ttip:~$ ./a.out
> > c sum: 35150012350464
> > p sum: 35184372088832
> > c sum: 35150012350464
> > p sum: 35184372088832
> > ...
>
> Thanks for the quick run, Anatoly.  Obviously I went the wrong way before
> on the code patching.  It seems we have more chance fixing this.
>
> --
> Peter Xu
>

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

[Peter Xu]

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

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

[hev]

Hi, Peter,

On Fri, Nov 18, 2022 at 2:29 AM Peter Xu <peterx@redhat.com> wrote:
>
> 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?

Sorry, I was wrong.

In this case, both VMAs are writable, the pte's writable bit is
cleared by pte_wrprotect. So if pte_mkdirty sets hardware writable bit
unconditionally, then there will be no way to catch writes to
implement COW.

I will try to explain how it works about pte write, dirty and
write-protect on LoongArch in the LoongArch mailing-list.

Regards,
Ray

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

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

[David Hildenbrand]

On 19.11.22 15:06, hev wrote:
> Hi, Peter,
> 
> On Fri, Nov 18, 2022 at 2:29 AM Peter Xu <peterx@redhat.com> wrote:
>>
>> 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?
> 
> Sorry, I was wrong.
> 
> In this case, both VMAs are writable, the pte's writable bit is
> cleared by pte_wrprotect. So if pte_mkdirty sets hardware writable bit
> unconditionally, then there will be no way to catch writes to
> implement COW.
> 
> I will try to explain how it works about pte write, dirty and
> write-protect on LoongArch in the LoongArch mailing-list.

Just to ask again,

is code like
	maybe_mkwrite(pte_mkdirty(entry), vma);

Like we have in copy_present_page(), wp_page_reuse(), wp_page_copy() ... 
broken on LoongArch of the VMA lacks VM_WRITE?

That would need *real* fixing, no hacks around that in other code areas.

-- 
Thanks,

David / dhildenb

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

[hev]

Hi, David,

>
> Just to ask again,
>
> is code like
>         maybe_mkwrite(pte_mkdirty(entry), vma);
>
> Like we have in copy_present_page(), wp_page_reuse(), wp_page_copy() ...
> broken on LoongArch of the VMA lacks VM_WRITE?

Sorry for the late reply. In this test case, The VMA doesn't lack VM_WRITE.

Regards,
Ray

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

[David Hildenbrand]

On 25.11.22 12:15, hev wrote:
> Hi, David,
> 
>>
>> Just to ask again,
>>
>> is code like
>>          maybe_mkwrite(pte_mkdirty(entry), vma);
>>
>> Like we have in copy_present_page(), wp_page_reuse(), wp_page_copy() ...
>> broken on LoongArch of the VMA lacks VM_WRITE?
> 
> Sorry for the late reply. In this test case, The VMA doesn't lack VM_WRITE.
> 

Let me ask again: is this a problem in all of the functions I mentioned 
above were we *could* lack VM_WRITE?

Because that needs fixing independently of this issue here.

-- 
Thanks,

David / dhildenb

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

[hev]

On Fri, Nov 25, 2022 at 7:17 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 25.11.22 12:15, hev wrote:
> > Hi, David,
> >
> >>
> >> Just to ask again,
> >>
> >> is code like
> >>          maybe_mkwrite(pte_mkdirty(entry), vma);
> >>
> >> Like we have in copy_present_page(), wp_page_reuse(), wp_page_copy() ...
> >> broken on LoongArch of the VMA lacks VM_WRITE?
> >
> > Sorry for the late reply. In this test case, The VMA doesn't lack VM_WRITE.
> >
>
> Let me ask again: is this a problem in all of the functions I mentioned
> above were we *could* lack VM_WRITE?
>
> Because that needs fixing independently of this issue here.

Oops. sorry I don't know.

Peter?

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

[Peter Xu]

[-- Attachment #1: Type: text/plain, Size: 1671 bytes --]

Hi, Anatoly (or/and Hev),

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,
> >
> > 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.
> >
> > Here is a simple and fast testcase (It may be helpful for sparc64):
> > https://gist.github.com/heiher/72919fae6b53f04cac606a9631100506
> > (assertion: c sum == 0)
> 
> Just tried on my sparc64 VM -  fixed vs old (non-patched) kernels...
> 
> fixed kernel (6.1.0-rc5) running ./a.out:
> mator@ttip:~$ ./a.out
> c sum: 0
> p sum: 35184372088832
> c sum: 0
> p sum: 35184372088832
> c sum: 0
> p sum: 35184372088832
> c sum: 0
> p sum: 35184372088832
> c sum: 0
> p sum: 35184372088832
> ...
> 
> old (non-patched) kernel (6.1.0-rc4) :
> mator@ttip:~$ ./a.out
> c sum: 35150012350464
> p sum: 35184372088832
> c sum: 35150012350464
> p sum: 35184372088832
> ...

I've got another patch attached that might be nicer to fix this same
problem for both archs but without dropping the dirty bit, could you help
check whether it works?

Hopefully the new patch could replace the other one (624a2c94f5b7 Partly
revert "mm/thp: carry over dirty bit when thp splits on pmd") in Andrew's
tree before it lands next rc1, and this new one should be applicable
directly to e.g. v6.0 tag (or need to have 624a2c94f5b7 reverted if on any
of Andrew's trees).

-- 
Peter Xu

[-- Attachment #2: 0001-mm-thp-Wr-protect-pte-after-mkdirty.patch --]
[-- Type: text/plain, Size: 2490 bytes --]

From e349b24573870ef50d0c1b3bf124e14f5dfe1fa5 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Mon, 21 Nov 2022 13:36:59 -0500
Subject: [PATCH] mm/thp: Wr-protect pte after mkdirty
Content-type: text/plain

Anatoly Pugachev reported sparc64 breakage on the patch:

https://lore.kernel.org/r/20221021160603.GA23307@u164.east.ru

Hev <r@hev.cc> also reported similar issue on loongarch:

(the original mail was private, but Anatoly copied the list here)
https://lore.kernel.org/r/CADxRZqxqb7f_WhMh=jweZP+ynf_JwGd-0VwbYgp4P+T0-AXosw@mail.gmail.com

Also Hev pointed out that the issue is having HW write bit set within the
pte_mkdirty() so the split pte can be written after split even if e.g. they
were shared by more than one processes, causing data corrupt.

Hev also tried to explain why loongarch set HW write bit in mkdirty:

https://lore.kernel.org/r/CAHirt9itKO_K_HPboXh5AyJtt16Zf0cD73PtHvM=na39u_ztxA@mail.gmail.com

One way to fix it is as what Huacai proposed here for loongarch:

https://lore.kernel.org/r/20221117042532.4064448-1-chenhuacai@loongson.cnn

Or more agressively, not sure whether (IMHO) we can remove the
"optimization" to grant HW write bit in pte_mkdirty() in both archs,
leaving set the write bit only in pte_mkwrite().

For now the simpler solution that'll work for all is we wr-protect after
pte_mkdirty(), so the HW write bit can be persistent after thp split.

Cc: Hev <r@hev.cc>
Cc: Andrew Morton <akpm@linux-foundation.org>
Reported-by: Anatoly Pugachev <matorola@gmail.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 mm/huge_memory.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index e5f5a1a00596..ae90b65f6121 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2191,13 +2191,18 @@ 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.37.3

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

[hev]

On Tue, Nov 22, 2022 at 2:55 AM Peter Xu <peterx@redhat.com> wrote:
>
> Hi, Anatoly (or/and Hev),
>
> 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,
> > >
> > > 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.
> > >
> > > Here is a simple and fast testcase (It may be helpful for sparc64):
> > > https://gist.github.com/heiher/72919fae6b53f04cac606a9631100506
> > > (assertion: c sum == 0)
> >
> > Just tried on my sparc64 VM -  fixed vs old (non-patched) kernels...
> >
> > fixed kernel (6.1.0-rc5) running ./a.out:
> > mator@ttip:~$ ./a.out
> > c sum: 0
> > p sum: 35184372088832
> > c sum: 0
> > p sum: 35184372088832
> > c sum: 0
> > p sum: 35184372088832
> > c sum: 0
> > p sum: 35184372088832
> > c sum: 0
> > p sum: 35184372088832
> > ...
> >
> > old (non-patched) kernel (6.1.0-rc4) :
> > mator@ttip:~$ ./a.out
> > c sum: 35150012350464
> > p sum: 35184372088832
> > c sum: 35150012350464
> > p sum: 35184372088832
> > ...
>
> I've got another patch attached that might be nicer to fix this same
> problem for both archs but without dropping the dirty bit, could you help
> check whether it works?

The tesecase PASSED with this patch and without:
 * "Partly revert "mm/thp: carry over dirty bit when thp splits on pmd"
 * "LoongArch: Set _PAGE_DIRTY only if _PAGE_WRITE is set in
{pmd,pte}_mkdirty()"

Ray

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

[Peter Xu]

On Fri, Nov 25, 2022 at 07:38:36PM +0800, hev wrote:
> On Tue, Nov 22, 2022 at 2:55 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > Hi, Anatoly (or/and Hev),
> >
> > 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,
> > > >
> > > > 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.
> > > >
> > > > Here is a simple and fast testcase (It may be helpful for sparc64):
> > > > https://gist.github.com/heiher/72919fae6b53f04cac606a9631100506
> > > > (assertion: c sum == 0)
> > >
> > > Just tried on my sparc64 VM -  fixed vs old (non-patched) kernels...
> > >
> > > fixed kernel (6.1.0-rc5) running ./a.out:
> > > mator@ttip:~$ ./a.out
> > > c sum: 0
> > > p sum: 35184372088832
> > > c sum: 0
> > > p sum: 35184372088832
> > > c sum: 0
> > > p sum: 35184372088832
> > > c sum: 0
> > > p sum: 35184372088832
> > > c sum: 0
> > > p sum: 35184372088832
> > > ...
> > >
> > > old (non-patched) kernel (6.1.0-rc4) :
> > > mator@ttip:~$ ./a.out
> > > c sum: 35150012350464
> > > p sum: 35184372088832
> > > c sum: 35150012350464
> > > p sum: 35184372088832
> > > ...
> >
> > I've got another patch attached that might be nicer to fix this same
> > problem for both archs but without dropping the dirty bit, could you help
> > check whether it works?
> 
> The tesecase PASSED with this patch and without:
>  * "Partly revert "mm/thp: carry over dirty bit when thp splits on pmd"
>  * "LoongArch: Set _PAGE_DIRTY only if _PAGE_WRITE is set in
> {pmd,pte}_mkdirty()"

My fault to not have noticed that the partly revert patch already landed
6.1-rc5, so it'll need to be another patch upon it.

I'll post a formal patch.  Thanks Hev.

-- 
Peter Xu