mapcount corruption regression

mapcount corruption regression

[Dan Williams]

Kirill, Willy, compound page experts,

I am seeking some debug ideas about the following splat:

BUG: Bad page state in process lt-pmem-ns  pfn:121a12
page:0000000051ef73f7 refcount:0 mapcount:-1024
mapping:0000000000000000 index:0x0 pfn:0x121a12
flags: 0x2ffff800000000()
raw: 002ffff800000000 dead000000000100 0000000000000000 0000000000000000
raw: 0000000000000000 ffff8a6914886b48 00000000fffffbff 0000000000000000
page dumped because: nonzero mapcount
[..]
CPU: 26 PID: 6127 Comm: lt-pmem-ns Tainted: G           OE     5.10.0-rc4+ #450
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
Call Trace:
 dump_stack+0x8b/0xb0
 bad_page.cold+0x63/0x94
 free_pcp_prepare+0x224/0x270
 free_unref_page+0x18/0xd0
 pud_free_pmd_page+0x146/0x160
 ioremap_pud_range+0xe3/0x350
 ioremap_page_range+0x108/0x160
 __ioremap_caller.constprop.0+0x174/0x2b0
 ? memremap+0x7a/0x110
 memremap+0x7a/0x110
 devm_memremap+0x53/0xa0
 pmem_attach_disk+0x4ed/0x530 [nd_pmem]

It triggers on v5.10-rc4 not on v5.9, but the bisect comes up with an
ambiguous result. I've run the bisect 3 times and landed on:

032c7ed95817 Merge tag 'arm64-upstream' of
git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux

...which does not touch anything near _mapcount. I suspect there is
something unique about the build that lines up the corruption to
happen or not happen.

The test is a simple namespace creation test that results in an
memremap() / ioremap() over several gigabytes of memory capacity. The
-1024 was interesting because that's the GUP_PIN_COUNTING_BIAS, but
that's the _refcount, I did not see any questionable changes to how
_mapcount is manipulated post v5.9. Problem should be reproducible by
running:

make -j TESTS="pmem-ns" check

...in qemu-kvm with some virtual pmem defined:

-object memory-backend-file,id=mem1,share,mem-path=${mem}1,size=$((mem_size+label_size))
-device nvdimm,memdev=mem1,id=nv1,label-size=${label_size}

...where ${mem}1 is a 128GB sparse file $mem_size is 127GB and
$label_size is 128KB.

Re: mapcount corruption regression

[Dan Williams]

On Mon, Nov 30, 2020 at 5:20 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Kirill, Willy, compound page experts,
>
> I am seeking some debug ideas about the following splat:
>
> BUG: Bad page state in process lt-pmem-ns  pfn:121a12

Looks to be a similar signature that Yi Zhang is seeing:

http://lore.kernel.org/r/51e938d1-aff7-0fa4-1a79-f77ac8bb2f8b@redhat.com

Re: mapcount corruption regression

[Matthew Wilcox]

On Mon, Nov 30, 2020 at 05:20:25PM -0800, Dan Williams wrote:
> Kirill, Willy, compound page experts,
> 
> I am seeking some debug ideas about the following splat:
> 
> BUG: Bad page state in process lt-pmem-ns  pfn:121a12
> page:0000000051ef73f7 refcount:0 mapcount:-1024
> mapping:0000000000000000 index:0x0 pfn:0x121a12

Mapcount of -1024 is the signature of:

#define PG_guard        0x00000400

(the bits are inverted, so this turns into 0xfffffbff which is reported
as -1024)

I assume you have debug_pagealloc enabled?

> flags: 0x2ffff800000000()
> raw: 002ffff800000000 dead000000000100 0000000000000000 0000000000000000
> raw: 0000000000000000 ffff8a6914886b48 00000000fffffbff 0000000000000000
> page dumped because: nonzero mapcount
> [..]
> CPU: 26 PID: 6127 Comm: lt-pmem-ns Tainted: G           OE     5.10.0-rc4+ #450
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> Call Trace:
>  dump_stack+0x8b/0xb0
>  bad_page.cold+0x63/0x94
>  free_pcp_prepare+0x224/0x270
>  free_unref_page+0x18/0xd0
>  pud_free_pmd_page+0x146/0x160
>  ioremap_pud_range+0xe3/0x350
>  ioremap_page_range+0x108/0x160
>  __ioremap_caller.constprop.0+0x174/0x2b0
>  ? memremap+0x7a/0x110
>  memremap+0x7a/0x110
>  devm_memremap+0x53/0xa0
>  pmem_attach_disk+0x4ed/0x530 [nd_pmem]
> 
> It triggers on v5.10-rc4 not on v5.9, but the bisect comes up with an
> ambiguous result. I've run the bisect 3 times and landed on:
> 
> 032c7ed95817 Merge tag 'arm64-upstream' of
> git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux
> 
> ...which does not touch anything near _mapcount. I suspect there is
> something unique about the build that lines up the corruption to
> happen or not happen.
> 
> The test is a simple namespace creation test that results in an
> memremap() / ioremap() over several gigabytes of memory capacity. The
> -1024 was interesting because that's the GUP_PIN_COUNTING_BIAS, but
> that's the _refcount, I did not see any questionable changes to how
> _mapcount is manipulated post v5.9. Problem should be reproducible by
> running:
> 
> make -j TESTS="pmem-ns" check
> 
> ...in qemu-kvm with some virtual pmem defined:
> 
> -object memory-backend-file,id=mem1,share,mem-path=${mem}1,size=$((mem_size+label_size))
> -device nvdimm,memdev=mem1,id=nv1,label-size=${label_size}
> 
> ...where ${mem}1 is a 128GB sparse file $mem_size is 127GB and
> $label_size is 128KB.
> 

Re: mapcount corruption regression

[Dan Williams]

On Mon, Nov 30, 2020 at 6:24 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Nov 30, 2020 at 05:20:25PM -0800, Dan Williams wrote:
> > Kirill, Willy, compound page experts,
> >
> > I am seeking some debug ideas about the following splat:
> >
> > BUG: Bad page state in process lt-pmem-ns  pfn:121a12
> > page:0000000051ef73f7 refcount:0 mapcount:-1024
> > mapping:0000000000000000 index:0x0 pfn:0x121a12
>
> Mapcount of -1024 is the signature of:
>
> #define PG_guard        0x00000400

Oh, thanks for that. I overlooked how mapcount is overloaded. Although
in v5.10-rc4 that value is:

#define PG_table        0x00000400

>
> (the bits are inverted, so this turns into 0xfffffbff which is reported
> as -1024)
>
> I assume you have debug_pagealloc enabled?

Added it, but no extra spew. I'll dig a bit more on how PG_table is
not being cleared in this case.

Re: mapcount corruption regression

[Matthew Wilcox]

On Tue, Dec 01, 2020 at 12:42:39PM -0800, Dan Williams wrote:
> On Mon, Nov 30, 2020 at 6:24 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Mon, Nov 30, 2020 at 05:20:25PM -0800, Dan Williams wrote:
> > > Kirill, Willy, compound page experts,
> > >
> > > I am seeking some debug ideas about the following splat:
> > >
> > > BUG: Bad page state in process lt-pmem-ns  pfn:121a12
> > > page:0000000051ef73f7 refcount:0 mapcount:-1024
> > > mapping:0000000000000000 index:0x0 pfn:0x121a12
> >
> > Mapcount of -1024 is the signature of:
> >
> > #define PG_guard        0x00000400
> 
> Oh, thanks for that. I overlooked how mapcount is overloaded. Although
> in v5.10-rc4 that value is:
> 
> #define PG_table        0x00000400

Ah, I was looking at -next, where Roman renumbered it.

I know UML had a problem where it was not clearing PG_table, but you
seem to be running on bare metal.  SuperH did too, but again, you're
not using SuperH.

> >
> > (the bits are inverted, so this turns into 0xfffffbff which is reported
> > as -1024)
> >
> > I assume you have debug_pagealloc enabled?
> 
> Added it, but no extra spew. I'll dig a bit more on how PG_table is
> not being cleared in this case.

I only asked about debug_pagealloc because that sets PG_guard.  Since
the problem is actually PG_table, it's not relevant.

Re: mapcount corruption regression

[Dan Williams]

On Tue, Dec 1, 2020 at 12:49 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Dec 01, 2020 at 12:42:39PM -0800, Dan Williams wrote:
> > On Mon, Nov 30, 2020 at 6:24 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Mon, Nov 30, 2020 at 05:20:25PM -0800, Dan Williams wrote:
> > > > Kirill, Willy, compound page experts,
> > > >
> > > > I am seeking some debug ideas about the following splat:
> > > >
> > > > BUG: Bad page state in process lt-pmem-ns  pfn:121a12
> > > > page:0000000051ef73f7 refcount:0 mapcount:-1024
> > > > mapping:0000000000000000 index:0x0 pfn:0x121a12
> > >
> > > Mapcount of -1024 is the signature of:
> > >
> > > #define PG_guard        0x00000400
> >
> > Oh, thanks for that. I overlooked how mapcount is overloaded. Although
> > in v5.10-rc4 that value is:
> >
> > #define PG_table        0x00000400
>
> Ah, I was looking at -next, where Roman renumbered it.
>
> I know UML had a problem where it was not clearing PG_table, but you
> seem to be running on bare metal.  SuperH did too, but again, you're
> not using SuperH.
>
> > >
> > > (the bits are inverted, so this turns into 0xfffffbff which is reported
> > > as -1024)
> > >
> > > I assume you have debug_pagealloc enabled?
> >
> > Added it, but no extra spew. I'll dig a bit more on how PG_table is
> > not being cleared in this case.
>
> I only asked about debug_pagealloc because that sets PG_guard.  Since
> the problem is actually PG_table, it's not relevant.

As a shot in the dark I reverted:

    b2b29d6d0119 mm: account PMD tables like PTE tables

...and the test passed.

Yi, do you see the same?

Re: mapcount corruption regression

[Matthew Wilcox]

On Tue, Dec 01, 2020 at 06:28:45PM -0800, Dan Williams wrote:
> On Tue, Dec 1, 2020 at 12:49 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Tue, Dec 01, 2020 at 12:42:39PM -0800, Dan Williams wrote:
> > > On Mon, Nov 30, 2020 at 6:24 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Mon, Nov 30, 2020 at 05:20:25PM -0800, Dan Williams wrote:
> > > > > Kirill, Willy, compound page experts,
> > > > >
> > > > > I am seeking some debug ideas about the following splat:
> > > > >
> > > > > BUG: Bad page state in process lt-pmem-ns  pfn:121a12
> > > > > page:0000000051ef73f7 refcount:0 mapcount:-1024
> > > > > mapping:0000000000000000 index:0x0 pfn:0x121a12
> > > >
> > > > Mapcount of -1024 is the signature of:
> > > >
> > > > #define PG_guard        0x00000400
> > >
> > > Oh, thanks for that. I overlooked how mapcount is overloaded. Although
> > > in v5.10-rc4 that value is:
> > >
> > > #define PG_table        0x00000400
> >
> > Ah, I was looking at -next, where Roman renumbered it.
> >
> > I know UML had a problem where it was not clearing PG_table, but you
> > seem to be running on bare metal.  SuperH did too, but again, you're
> > not using SuperH.
> >
> > > >
> > > > (the bits are inverted, so this turns into 0xfffffbff which is reported
> > > > as -1024)
> > > >
> > > > I assume you have debug_pagealloc enabled?
> > >
> > > Added it, but no extra spew. I'll dig a bit more on how PG_table is
> > > not being cleared in this case.
> >
> > I only asked about debug_pagealloc because that sets PG_guard.  Since
> > the problem is actually PG_table, it's not relevant.
> 
> As a shot in the dark I reverted:
> 
>     b2b29d6d0119 mm: account PMD tables like PTE tables
> 
> ...and the test passed.

That's not really surprising ... you're still freeing PMD tables without
calling the destructor, which means that you're leaking ptlocks on
configs that can't embed the ptlock in the struct page.

I suppose it shows that you're leaking a PMD table rather than a PTE
table, so that might help track it down.  Checking for PG_table in
free_unref_page() and calling show_stack() will probably help more.

Re: mapcount corruption regression

[Dan Williams]

On Tue, Dec 1, 2020 at 7:43 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Dec 01, 2020 at 06:28:45PM -0800, Dan Williams wrote:
> > On Tue, Dec 1, 2020 at 12:49 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Tue, Dec 01, 2020 at 12:42:39PM -0800, Dan Williams wrote:
> > > > On Mon, Nov 30, 2020 at 6:24 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > >
> > > > > On Mon, Nov 30, 2020 at 05:20:25PM -0800, Dan Williams wrote:
> > > > > > Kirill, Willy, compound page experts,
> > > > > >
> > > > > > I am seeking some debug ideas about the following splat:
> > > > > >
> > > > > > BUG: Bad page state in process lt-pmem-ns  pfn:121a12
> > > > > > page:0000000051ef73f7 refcount:0 mapcount:-1024
> > > > > > mapping:0000000000000000 index:0x0 pfn:0x121a12
> > > > >
> > > > > Mapcount of -1024 is the signature of:
> > > > >
> > > > > #define PG_guard        0x00000400
> > > >
> > > > Oh, thanks for that. I overlooked how mapcount is overloaded. Although
> > > > in v5.10-rc4 that value is:
> > > >
> > > > #define PG_table        0x00000400
> > >
> > > Ah, I was looking at -next, where Roman renumbered it.
> > >
> > > I know UML had a problem where it was not clearing PG_table, but you
> > > seem to be running on bare metal.  SuperH did too, but again, you're
> > > not using SuperH.
> > >
> > > > >
> > > > > (the bits are inverted, so this turns into 0xfffffbff which is reported
> > > > > as -1024)
> > > > >
> > > > > I assume you have debug_pagealloc enabled?
> > > >
> > > > Added it, but no extra spew. I'll dig a bit more on how PG_table is
> > > > not being cleared in this case.
> > >
> > > I only asked about debug_pagealloc because that sets PG_guard.  Since
> > > the problem is actually PG_table, it's not relevant.
> >
> > As a shot in the dark I reverted:
> >
> >     b2b29d6d0119 mm: account PMD tables like PTE tables
> >
> > ...and the test passed.
>
> That's not really surprising ... you're still freeing PMD tables without
> calling the destructor, which means that you're leaking ptlocks on
> configs that can't embed the ptlock in the struct page.

Ok, so potentially this new tracking is highlighting a long standing
bug that was previously silent. That would explain the ambiguous
bisect results.

> I suppose it shows that you're leaking a PMD table rather than a PTE
> table, so that might help track it down.  Checking for PG_table in
> free_unref_page() and calling show_stack() will probably help more.

Will do.

Re: mapcount corruption regression

[Dan Williams]

On Tue, Dec 1, 2020 at 9:07 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Tue, Dec 1, 2020 at 7:43 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Tue, Dec 01, 2020 at 06:28:45PM -0800, Dan Williams wrote:
> > > On Tue, Dec 1, 2020 at 12:49 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Tue, Dec 01, 2020 at 12:42:39PM -0800, Dan Williams wrote:
> > > > > On Mon, Nov 30, 2020 at 6:24 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > >
> > > > > > On Mon, Nov 30, 2020 at 05:20:25PM -0800, Dan Williams wrote:
> > > > > > > Kirill, Willy, compound page experts,
> > > > > > >
> > > > > > > I am seeking some debug ideas about the following splat:
> > > > > > >
> > > > > > > BUG: Bad page state in process lt-pmem-ns  pfn:121a12
> > > > > > > page:0000000051ef73f7 refcount:0 mapcount:-1024
> > > > > > > mapping:0000000000000000 index:0x0 pfn:0x121a12
> > > > > >
> > > > > > Mapcount of -1024 is the signature of:
> > > > > >
> > > > > > #define PG_guard        0x00000400
> > > > >
> > > > > Oh, thanks for that. I overlooked how mapcount is overloaded. Although
> > > > > in v5.10-rc4 that value is:
> > > > >
> > > > > #define PG_table        0x00000400
> > > >
> > > > Ah, I was looking at -next, where Roman renumbered it.
> > > >
> > > > I know UML had a problem where it was not clearing PG_table, but you
> > > > seem to be running on bare metal.  SuperH did too, but again, you're
> > > > not using SuperH.
> > > >
> > > > > >
> > > > > > (the bits are inverted, so this turns into 0xfffffbff which is reported
> > > > > > as -1024)
> > > > > >
> > > > > > I assume you have debug_pagealloc enabled?
> > > > >
> > > > > Added it, but no extra spew. I'll dig a bit more on how PG_table is
> > > > > not being cleared in this case.
> > > >
> > > > I only asked about debug_pagealloc because that sets PG_guard.  Since
> > > > the problem is actually PG_table, it's not relevant.
> > >
> > > As a shot in the dark I reverted:
> > >
> > >     b2b29d6d0119 mm: account PMD tables like PTE tables
> > >
> > > ...and the test passed.
> >
> > That's not really surprising ... you're still freeing PMD tables without
> > calling the destructor, which means that you're leaking ptlocks on
> > configs that can't embed the ptlock in the struct page.
>
> Ok, so potentially this new tracking is highlighting a long standing
> bug that was previously silent. That would explain the ambiguous
> bisect results.
>
> > I suppose it shows that you're leaking a PMD table rather than a PTE
> > table, so that might help track it down.  Checking for PG_table in
> > free_unref_page() and calling show_stack() will probably help more.
>
> Will do.

Thanks for the pointers Willy this fix below tests ok and looks
correct to me given the history:

diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index dfd82f51ba66..7ed99314dcdf 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -829,6 +829,7 @@ int pud_free_pmd_page(pud_t *pud, unsigned long addr)
        }

        free_page((unsigned long)pmd_sv);
+       pgtable_pmd_page_dtor(virt_to_page(pmd));
        free_page((unsigned long)pmd);

        return 1;

In 2013 Kirill noticed that he missed a pmd page table free site:

    c283610e44ec x86, mm: do not leak page->ptl for pmd page tables

In 2018 Toshi added a new pmd page table free site without the destructor:

    28ee90fe6048 x86/mm: implement free pmd/pte page interfaces

In 2020 Willy adds PG_table accounting that flags the missing
pgtable_pmd_page_dtor()

Yi, I would appreciate a confirmation that the fix works for you.

Re: mapcount corruption regression

[Yi Zhang]

Hi Dan
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index dfd82f51ba66..7ed99314dcdf 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -829,6 +829,7 @@ int pud_free_pmd_page(pud_t *pud, unsigned long addr)
>          }
>
>          free_page((unsigned long)pmd_sv);
> +       pgtable_pmd_page_dtor(virt_to_page(pmd));
>          free_page((unsigned long)pmd);
>
>          return 1;
>
> In 2013 Kirill noticed that he missed a pmd page table free site:
>
>      c283610e44ec x86, mm: do not leak page->ptl for pmd page tables
>
> In 2018 Toshi added a new pmd page table free site without the destructor:
>
>      28ee90fe6048 x86/mm: implement free pmd/pte page interfaces
>
> In 2020 Willy adds PG_table accounting that flags the missing
> pgtable_pmd_page_dtor()
>
> Yi, I would appreciate a confirmation that the fix works for you.
>
I applied the patch to v5.10-rc3 ~ v5.10-rc6, and cannot reproduce this 
issue with my regression test now, feel free to add:
Tested-by: Yi Zhang <yi.zhang@redhat.com>


Thanks
Yi