[PATCH] fork: Improve error message for corrupted page tables

[PATCH] fork: Improve error message for corrupted page tables

[Sai Praneeth Prakhya]

When a user process exits, the kernel cleans up the mm_struct of the user
process and during cleanup, check_mm() checks the page tables of the user
process for corruption (E.g: unexpected page flags set/cleared). For
corrupted page tables, the error message printed by check_mm() isn't very
clear as it prints the loop index instead of page table type (E.g: Resident
file mapping pages vs Resident shared memory pages). Hence, improve the
error message so that it's more informative.

Without patch:
--------------
[  204.836425] mm/pgtable-generic.c:29: bad p4d 0000000089eb4e92(800000025f941467)
[  204.836544] BUG: Bad rss-counter state mm:00000000f75895ea idx:0 val:2
[  204.836615] BUG: Bad rss-counter state mm:00000000f75895ea idx:1 val:5
[  204.836685] BUG: non-zero pgtables_bytes on freeing mm: 20480

With patch:
-----------
[   69.815453] mm/pgtable-generic.c:29: bad p4d 0000000084653642(800000025ca37467)
[   69.815872] BUG: Bad rss-counter state mm:00000000014a6c03 type:MM_FILEPAGES val:2
[   69.815962] BUG: Bad rss-counter state mm:00000000014a6c03 type:MM_ANONPAGES val:5
[   69.816050] BUG: non-zero pgtables_bytes on freeing mm: 20480

Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Suggested-by/Acked-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
---
 include/linux/mm_types_task.h | 7 +++++++
 kernel/fork.c                 | 4 ++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/mm_types_task.h b/include/linux/mm_types_task.h
index d7016dcb245e..881f4ea3a1b5 100644
--- a/include/linux/mm_types_task.h
+++ b/include/linux/mm_types_task.h
@@ -44,6 +44,13 @@ enum {
 	NR_MM_COUNTERS
 };
 
+static const char * const resident_page_types[NR_MM_COUNTERS] = {
+	"MM_FILEPAGES",
+	"MM_ANONPAGES",
+	"MM_SWAPENTS",
+	"MM_SHMEMPAGES",
+};
+
 #if USE_SPLIT_PTE_PTLOCKS && defined(CONFIG_MMU)
 #define SPLIT_RSS_COUNTING
 /* per-thread cached information, */
diff --git a/kernel/fork.c b/kernel/fork.c
index 2852d0e76ea3..6aef5842d4e0 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -649,8 +649,8 @@ static void check_mm(struct mm_struct *mm)
 		long x = atomic_long_read(&mm->rss_stat.count[i]);
 
 		if (unlikely(x))
-			printk(KERN_ALERT "BUG: Bad rss-counter state "
-					  "mm:%p idx:%d val:%ld\n", mm, i, x);
+			pr_alert("BUG: Bad rss-counter state mm:%p type:%s val:%ld\n",
+				 mm, resident_page_types[i], x);
 	}
 
 	if (mm_pgtables_bytes(mm))
-- 
2.19.1

Re: [PATCH] fork: Improve error message for corrupted page tables

[Andrew Morton]

On Tue, 30 Jul 2019 15:18:20 -0700 Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> wrote:

> When a user process exits, the kernel cleans up the mm_struct of the user
> process and during cleanup, check_mm() checks the page tables of the user
> process for corruption (E.g: unexpected page flags set/cleared). For
> corrupted page tables, the error message printed by check_mm() isn't very
> clear as it prints the loop index instead of page table type (E.g: Resident
> file mapping pages vs Resident shared memory pages). Hence, improve the
> error message so that it's more informative.
> 
> Without patch:
> --------------
> [  204.836425] mm/pgtable-generic.c:29: bad p4d 0000000089eb4e92(800000025f941467)
> [  204.836544] BUG: Bad rss-counter state mm:00000000f75895ea idx:0 val:2
> [  204.836615] BUG: Bad rss-counter state mm:00000000f75895ea idx:1 val:5
> [  204.836685] BUG: non-zero pgtables_bytes on freeing mm: 20480
> 
> With patch:
> -----------
> [   69.815453] mm/pgtable-generic.c:29: bad p4d 0000000084653642(800000025ca37467)
> [   69.815872] BUG: Bad rss-counter state mm:00000000014a6c03 type:MM_FILEPAGES val:2
> [   69.815962] BUG: Bad rss-counter state mm:00000000014a6c03 type:MM_ANONPAGES val:5
> [   69.816050] BUG: non-zero pgtables_bytes on freeing mm: 20480

Seems useful.

> --- a/include/linux/mm_types_task.h
> +++ b/include/linux/mm_types_task.h
> @@ -44,6 +44,13 @@ enum {
>  	NR_MM_COUNTERS
>  };
>  
> +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> +	"MM_FILEPAGES",
> +	"MM_ANONPAGES",
> +	"MM_SWAPENTS",
> +	"MM_SHMEMPAGES",
> +};

But please let's not put this in a header file.  We're asking the
compiler to put a copy of all of this into every compilation unit which
includes the header.  Presumably the compiler is smart enough not to
do that, but it's not good practice.

Re: [PATCH] fork: Improve error message for corrupted page tables

[Sai Praneeth Prakhya]

> > With patch:
> > -----------
> > [   69.815453] mm/pgtable-generic.c:29: bad p4d
> > 0000000084653642(800000025ca37467)
> > [   69.815872] BUG: Bad rss-counter state mm:00000000014a6c03
> > type:MM_FILEPAGES val:2
> > [   69.815962] BUG: Bad rss-counter state mm:00000000014a6c03
> > type:MM_ANONPAGES val:5
> > [   69.816050] BUG: non-zero pgtables_bytes on freeing mm: 20480
> 
> Seems useful.
> 
> > --- a/include/linux/mm_types_task.h
> > +++ b/include/linux/mm_types_task.h
> > @@ -44,6 +44,13 @@ enum {
> >  	NR_MM_COUNTERS
> >  };
> >  
> > +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> > +	"MM_FILEPAGES",
> > +	"MM_ANONPAGES",
> > +	"MM_SWAPENTS",
> > +	"MM_SHMEMPAGES",
> > +};
> 
> But please let's not put this in a header file.  We're asking the
> compiler to put a copy of all of this into every compilation unit which
> includes the header.  Presumably the compiler is smart enough not to
> do that, but it's not good practice.

Thanks for the explanation. Makes sense to me.

Just wanted to check before sending V2,
Is it OK if I add this to kernel/fork.c? or do you have something else in
mind?

Regards,
Sai

Re: [PATCH] fork: Improve error message for corrupted page tables

[Andrew Morton]

On Wed, 31 Jul 2019 15:36:49 -0700 Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> wrote:

> > > +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> > > +	"MM_FILEPAGES",
> > > +	"MM_ANONPAGES",
> > > +	"MM_SWAPENTS",
> > > +	"MM_SHMEMPAGES",
> > > +};
> > 
> > But please let's not put this in a header file.  We're asking the
> > compiler to put a copy of all of this into every compilation unit which
> > includes the header.  Presumably the compiler is smart enough not to
> > do that, but it's not good practice.
> 
> Thanks for the explanation. Makes sense to me.
> 
> Just wanted to check before sending V2,
> Is it OK if I add this to kernel/fork.c? or do you have something else in
> mind?

I was thinking somewhere like mm/util.c so the array could be used by
other code.  But it seems there is no such code.  Perhaps it's best to
just leave fork.c as it is now.

Re: [PATCH] fork: Improve error message for corrupted page tables

[Anshuman Khandual]

On 07/31/2019 03:48 AM, Sai Praneeth Prakhya wrote:
> When a user process exits, the kernel cleans up the mm_struct of the user
> process and during cleanup, check_mm() checks the page tables of the user
> process for corruption (E.g: unexpected page flags set/cleared). For
> corrupted page tables, the error message printed by check_mm() isn't very
> clear as it prints the loop index instead of page table type (E.g: Resident
> file mapping pages vs Resident shared memory pages). Hence, improve the
> error message so that it's more informative.

The loop index in check_mm() also happens to be the index in rss_stat[] which
represents individual memory type stats. But you are right, index value here
in the print does not make any sense.

> 
> Without patch:
> --------------
> [  204.836425] mm/pgtable-generic.c:29: bad p4d 0000000089eb4e92(800000025f941467)
> [  204.836544] BUG: Bad rss-counter state mm:00000000f75895ea idx:0 val:2
> [  204.836615] BUG: Bad rss-counter state mm:00000000f75895ea idx:1 val:5
> [  204.836685] BUG: non-zero pgtables_bytes on freeing mm: 20480
> 
> With patch:
> -----------
> [   69.815453] mm/pgtable-generic.c:29: bad p4d 0000000084653642(800000025ca37467)
> [   69.815872] BUG: Bad rss-counter state mm:00000000014a6c03 type:MM_FILEPAGES val:2
> [   69.815962] BUG: Bad rss-counter state mm:00000000014a6c03 type:MM_ANONPAGES val:5
> [   69.816050] BUG: non-zero pgtables_bytes on freeing mm: 20480

Yes, this is definitely better.

> 
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Suggested-by/Acked-by: Dave Hansen <dave.hansen@intel.com>

Though I am not sure, should the above be two separate lines instead ?

> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> ---
>  include/linux/mm_types_task.h | 7 +++++++
>  kernel/fork.c                 | 4 ++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mm_types_task.h b/include/linux/mm_types_task.h
> index d7016dcb245e..881f4ea3a1b5 100644
> --- a/include/linux/mm_types_task.h
> +++ b/include/linux/mm_types_task.h
> @@ -44,6 +44,13 @@ enum {
>  	NR_MM_COUNTERS
>  };
>  
> +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> +	"MM_FILEPAGES",
> +	"MM_ANONPAGES",
> +	"MM_SWAPENTS",
> +	"MM_SHMEMPAGES",
> +};

Should index them to match respective typo macros.

	[MM_FILEPAGES] = "MM_FILEPAGES",
	[MM_ANONPAGES] = "MM_ANONPAGES",
	[MM_SWAPENTS] = "MM_SWAPENTS",
	[MM_SHMEMPAGES] = "MM_SHMEMPAGES",

> +
>  #if USE_SPLIT_PTE_PTLOCKS && defined(CONFIG_MMU)
>  #define SPLIT_RSS_COUNTING
>  /* per-thread cached information, */
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 2852d0e76ea3..6aef5842d4e0 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -649,8 +649,8 @@ static void check_mm(struct mm_struct *mm)
>  		long x = atomic_long_read(&mm->rss_stat.count[i]);
>  
>  		if (unlikely(x))
> -			printk(KERN_ALERT "BUG: Bad rss-counter state "
> -					  "mm:%p idx:%d val:%ld\n", mm, i, x);
> +			pr_alert("BUG: Bad rss-counter state mm:%p type:%s val:%ld\n",
> +				 mm, resident_page_types[i], x);
It changes the print function as well, though very minor change but perhaps
mention that in the commit message ?

RE: [PATCH] fork: Improve error message for corrupted page tables

[Prakhya, Sai Praneeth]

> > > > +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> > > > +	"MM_FILEPAGES",
> > > > +	"MM_ANONPAGES",
> > > > +	"MM_SWAPENTS",
> > > > +	"MM_SHMEMPAGES",
> > > > +};
> > >
> > > But please let's not put this in a header file.  We're asking the
> > > compiler to put a copy of all of this into every compilation unit
> > > which includes the header.  Presumably the compiler is smart enough
> > > not to do that, but it's not good practice.
> >
> > Thanks for the explanation. Makes sense to me.
> >
> > Just wanted to check before sending V2, Is it OK if I add this to
> > kernel/fork.c? or do you have something else in mind?
> 
> I was thinking somewhere like mm/util.c so the array could be used by other
> code.  But it seems there is no such code.  Perhaps it's best to just leave fork.c as
> it is now.

Ok, so does that mean have the struct in header file itself?
Sorry! for too many questions. I wanted to check with you before changing 
because it's *the* fork.c file (I presume random changes will not be encouraged here)

I am not yet clear on what's the right thing to do here :(
So, could you please help me in deciding.

Regards,
Sai

RE: [PATCH] fork: Improve error message for corrupted page tables

[Prakhya, Sai Praneeth]

> >
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Suggested-by/Acked-by: Dave Hansen <dave.hansen@intel.com>
> 
> Though I am not sure, should the above be two separate lines instead ?

Sure! Will split them in V2.

> 
> > Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> > ---
> >  include/linux/mm_types_task.h | 7 +++++++
> >  kernel/fork.c                 | 4 ++--
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/mm_types_task.h
> > b/include/linux/mm_types_task.h index d7016dcb245e..881f4ea3a1b5
> > 100644
> > --- a/include/linux/mm_types_task.h
> > +++ b/include/linux/mm_types_task.h
> > @@ -44,6 +44,13 @@ enum {
> >  	NR_MM_COUNTERS
> >  };
> >
> > +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> > +	"MM_FILEPAGES",
> > +	"MM_ANONPAGES",
> > +	"MM_SWAPENTS",
> > +	"MM_SHMEMPAGES",
> > +};
> 
> Should index them to match respective typo macros.
> 
> 	[MM_FILEPAGES] = "MM_FILEPAGES",
> 	[MM_ANONPAGES] = "MM_ANONPAGES",
> 	[MM_SWAPENTS] = "MM_SWAPENTS",
> 	[MM_SHMEMPAGES] = "MM_SHMEMPAGES",

Sure! Will change it.

> > +
> >  #if USE_SPLIT_PTE_PTLOCKS && defined(CONFIG_MMU)  #define
> > SPLIT_RSS_COUNTING
> >  /* per-thread cached information, */
> > diff --git a/kernel/fork.c b/kernel/fork.c index
> > 2852d0e76ea3..6aef5842d4e0 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -649,8 +649,8 @@ static void check_mm(struct mm_struct *mm)
> >  		long x = atomic_long_read(&mm->rss_stat.count[i]);
> >
> >  		if (unlikely(x))
> > -			printk(KERN_ALERT "BUG: Bad rss-counter state "
> > -					  "mm:%p idx:%d val:%ld\n", mm, i, x);
> > +			pr_alert("BUG: Bad rss-counter state mm:%p type:%s
> val:%ld\n",
> > +				 mm, resident_page_types[i], x);
> It changes the print function as well, though very minor change but perhaps
> mention that in the commit message ?

Sure! Will mention it in V2.
I have changed printk() to pr_alert() because the other message in check_mm() uses pr_alert().

Regards,
Sai

Re: [PATCH] fork: Improve error message for corrupted page tables

[Vlastimil Babka]

On 8/2/19 8:46 AM, Prakhya, Sai Praneeth wrote:
>>>>> +static const char * const resident_page_types[NR_MM_COUNTERS] = {
>>>>> +	"MM_FILEPAGES",
>>>>> +	"MM_ANONPAGES",
>>>>> +	"MM_SWAPENTS",
>>>>> +	"MM_SHMEMPAGES",
>>>>> +};
>>>>
>>>> But please let's not put this in a header file.  We're asking the
>>>> compiler to put a copy of all of this into every compilation unit
>>>> which includes the header.  Presumably the compiler is smart enough
>>>> not to do that, but it's not good practice.
>>>
>>> Thanks for the explanation. Makes sense to me.
>>>
>>> Just wanted to check before sending V2, Is it OK if I add this to
>>> kernel/fork.c? or do you have something else in mind?
>>
>> I was thinking somewhere like mm/util.c so the array could be used by other
>> code.  But it seems there is no such code.  Perhaps it's best to just leave fork.c as
>> it is now.
> 
> Ok, so does that mean have the struct in header file itself?

If the struct definition (including the string values) was in mm/util.c,
there would have to be a declaration in a header. If it's in fork.c with
the only users, there doesn't need to be separate declaration in a header.

> Sorry! for too many questions. I wanted to check with you before changing 
> because it's *the* fork.c file (I presume random changes will not be encouraged here)
> 
> I am not yet clear on what's the right thing to do here :(
> So, could you please help me in deciding.

fork.c should be fine, IMHO

> Regards,
> Sai
> 

Re: [PATCH] fork: Improve error message for corrupted page tables

[Sai Praneeth Prakhya]

On Mon, 2019-08-05 at 15:28 +0200, Vlastimil Babka wrote:
> On 8/2/19 8:46 AM, Prakhya, Sai Praneeth wrote:
> > > > > > +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> > > > > > +	"MM_FILEPAGES",
> > > > > > +	"MM_ANONPAGES",
> > > > > > +	"MM_SWAPENTS",
> > > > > > +	"MM_SHMEMPAGES",
> > > > > > +};
> > > > > 
> > > > > But please let's not put this in a header file.  We're asking the
> > > > > compiler to put a copy of all of this into every compilation unit
> > > > > which includes the header.  Presumably the compiler is smart enough
> > > > > not to do that, but it's not good practice.
> > > > 
> > > > Thanks for the explanation. Makes sense to me.
> > > > 
> > > > Just wanted to check before sending V2, Is it OK if I add this to
> > > > kernel/fork.c? or do you have something else in mind?
> > > 
> > > I was thinking somewhere like mm/util.c so the array could be used by
> > > other
> > > code.  But it seems there is no such code.  Perhaps it's best to just
> > > leave fork.c as
> > > it is now.
> > 
> > Ok, so does that mean have the struct in header file itself?
> 
> If the struct definition (including the string values) was in mm/util.c,
> there would have to be a declaration in a header. If it's in fork.c with
> the only users, there doesn't need to be separate declaration in a header.

Makes sense.

> 
> > Sorry! for too many questions. I wanted to check with you before changing 
> > because it's *the* fork.c file (I presume random changes will not be
> > encouraged here)
> > 
> > I am not yet clear on what's the right thing to do here :(
> > So, could you please help me in deciding.
> 
> fork.c should be fine, IMHO

I was leaning to add struct definition in fork.c as well but just wanted to
check with Andrew before posting V2.

Thanks for the reply though :)

Regards,
Sai