Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1578364 > unrolled thread

Re: [PATCH] x86/mm/ptdump: Fix soft lockup in page table walker.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86/mm/ptdump: Fix soft lockup in page table walker. Dmitry Vyukov <dvyukov@google.com> - 2017-02-10 11:30 +0100
    Re: [PATCH] x86/mm/ptdump: Fix soft lockup in page table walker. Thomas Gleixner <tglx@linutronix.de> - 2017-02-10 12:40 +0100
      Re: [PATCH] x86/mm/ptdump: Fix soft lockup in page table walker. Dmitry Vyukov <dvyukov@google.com> - 2017-02-10 14:30 +0100
        Re: [PATCH] x86/mm/ptdump: Fix soft lockup in page table walker. Dmitry Vyukov <dvyukov@google.com> - 2017-02-10 15:20 +0100
        Re: [PATCH] x86/mm/ptdump: Fix soft lockup in page table walker. Mark Rutland <mark.rutland@arm.com> - 2017-02-10 15:40 +0100
          Re: [PATCH] x86/mm/ptdump: Fix soft lockup in page table walker. Mark Rutland <mark.rutland@arm.com> - 2017-02-10 15:50 +0100
        Re: [PATCH] x86/mm/ptdump: Fix soft lockup in page table walker. Mark Rutland <mark.rutland@arm.com> - 2017-02-10 16:00 +0100

#1578364 — Re: [PATCH] x86/mm/ptdump: Fix soft lockup in page table walker.

On Fri, Feb 10, 2017 at 10:54 AM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> CONFIG_KASAN=y needs a lot of virtual memory mapped for its shadow.
> In that case ptdump_walk_pgd_level_core() takes a lot of time to
> walk across all page tables and doing this without
> a rescheduling causes soft lockups:
>
>  NMI watchdog: BUG: soft lockup - CPU#3 stuck for 23s! [swapper/0:1]
>  ...
>  Call Trace:
>   ptdump_walk_pgd_level_core+0x40c/0x550
>   ptdump_walk_pgd_level_checkwx+0x17/0x20
>   mark_rodata_ro+0x13b/0x150
>   kernel_init+0x2f/0x120
>   ret_from_fork+0x2c/0x40
>
> I guess that this issue might arise even without KASAN on a huge
> machines with several terabytes of RAM.
>
> Stick cond_resched() in pgd loop to fix this.
>
> Reported-by: Tobias Regnery <tobias.regnery@gmail.com>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: <stable@vger.kernel.org>
> ---
>  arch/x86/mm/dump_pagetables.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
> index ea9c49a..8aa6bea 100644
> --- a/arch/x86/mm/dump_pagetables.c
> +++ b/arch/x86/mm/dump_pagetables.c
> @@ -15,6 +15,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/mm.h>
>  #include <linux/init.h>
> +#include <linux/sched.h>
>  #include <linux/seq_file.h>
>
>  #include <asm/pgtable.h>
> @@ -406,6 +407,7 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
>                 } else
>                         note_page(m, &st, __pgprot(0), 1);
>
> +               cond_resched();


This is the right thing to do per se, but I am concerned that now
people will just suffers from slow boot (it can take literally
minutes) and will not realize the root cause nor that it's fixable
(e.g. with rodata=n) and will probably just blame KASAN for slowness.

Could we default this rodata check to n under KASAN? Or at least print
some explanatory warning message before doing marking rodata (it
should be printed right before "hang", so if you stare at it for a
minute during each boot you realize that it may be related)? Or
something along these lines. FWIW in my builds I just always disable
the check.

[toc] | [next] | [standalone]

#1578406

On Fri, 10 Feb 2017, Dmitry Vyukov wrote:
> This is the right thing to do per se, but I am concerned that now
> people will just suffers from slow boot (it can take literally
> minutes) and will not realize the root cause nor that it's fixable
> (e.g. with rodata=n) and will probably just blame KASAN for slowness.
> 
> Could we default this rodata check to n under KASAN? Or at least print
> some explanatory warning message before doing marking rodata (it
> should be printed right before "hang", so if you stare at it for a
> minute during each boot you realize that it may be related)? Or
> something along these lines. FWIW in my builds I just always disable
> the check.

That certainly makes sense and we emit such warnings in other places
already (lockdep, trace_printk ...)

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1578446

On Fri, Feb 10, 2017 at 1:15 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
>
>
> On 02/10/2017 02:18 PM, Thomas Gleixner wrote:
>> On Fri, 10 Feb 2017, Dmitry Vyukov wrote:
>>> This is the right thing to do per se, but I am concerned that now
>>> people will just suffers from slow boot (it can take literally
>>> minutes) and will not realize the root cause nor that it's fixable
>>> (e.g. with rodata=n) and will probably just blame KASAN for slowness.
>>>
>>> Could we default this rodata check to n under KASAN? Or at least print
>>> some explanatory warning message before doing marking rodata (it
>>> should be printed right before "hang", so if you stare at it for a
>>> minute during each boot you realize that it may be related)? Or
>>> something along these lines. FWIW in my builds I just always disable
>>> the check.
>>
>> That certainly makes sense and we emit such warnings in other places
>> already (lockdep, trace_printk ...)
>>
>
> Agreed, but perhaps it would be better to make this code faster for KASAN=y?
> The main problem here is that we have many pgd entries containing kasan_zero_pud values
> and ptdump walker checks kasan_zero_pud many times.
> Instead, we could check it only once and skip further kasan_zero_pud's.
>
> I can't say I like this hack very much, but it wins me almost 20 seconds of boot time.
> Any objections?


Now I remember that we already discussed it in this thread:
https://lkml.org/lkml/2016/11/8/775

Andrey, you proposed:

"I didn't look at any code, but we probably could can remember last
visited pgd and skip next pgd if it's the same as previous."

Do you still think it's a good idea?
Walking the same pgd multiple times does not make sense (right?). And
it could probably speedup non-kasan builds to some degree in some
contexts. And the code will be free of additional ifdefs.



> diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
> index 8aa6bea..0fbae1d 100644
> --- a/arch/x86/mm/dump_pagetables.c
> +++ b/arch/x86/mm/dump_pagetables.c
> @@ -13,6 +13,7 @@
>   */
>
>  #include <linux/debugfs.h>
> +#include <linux/kasan.h>
>  #include <linux/mm.h>
>  #include <linux/init.h>
>  #include <linux/sched.h>
> @@ -121,6 +122,30 @@ static struct addr_marker address_markers[] = {
>                         seq_printf(m, fmt, ##args);             \
>  })
>
> +
> +#ifdef CONFIG_KASAN
> +static bool kasan_pgd_checked(pgd_t pgd, bool checkwx)
> +{
> +       static bool kasan_zero_pgd_checked = false;
> +       pgd_t kasan_zero_pgd = __pgd(__pa(kasan_zero_pud) | _PAGE_TABLE);
> +
> +       if (!checkwx)
> +               return false;
> +
> +       if (pgd_val(pgd) == pgd_val(kasan_zero_pgd)) {
> +               if (kasan_zero_pgd_checked)
> +                       return true;
> +               kasan_zero_pgd_checked = true;
> +       }
> +       return false;
> +}
> +#else
> +static inline bool kasan_pgd_checked(pgd_t pgd, bool checkwx)
> +{
> +       return false;
> +}
> +#endif
> +
>  /*
>   * Print a readable form of a pgprot_t to the seq_file
>   */
> @@ -396,7 +421,8 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
>
>         for (i = 0; i < PTRS_PER_PGD; i++) {
>                 st.current_address = normalize_addr(i * PGD_LEVEL_MULT);
> -               if (!pgd_none(*start) && !is_hypervisor_range(i)) {
> +               if (!pgd_none(*start) && !is_hypervisor_range(i) &&
> +                               !kasan_pgd_checked(*start, checkwx)) {
>                         if (pgd_large(*start) || !pgd_present(*start)) {
>                                 prot = pgd_flags(*start);
>                                 note_page(m, &st, __pgprot(prot), 1);
>

[toc] | [prev] | [next] | [standalone]

#1578491

On Fri, Feb 10, 2017 at 2:56 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> On 02/10/2017 04:02 PM, Dmitry Vyukov wrote:
>> On Fri, Feb 10, 2017 at 1:15 PM, Andrey Ryabinin
>> <aryabinin@virtuozzo.com> wrote:
>>>
>>>
>>> On 02/10/2017 02:18 PM, Thomas Gleixner wrote:
>>>> On Fri, 10 Feb 2017, Dmitry Vyukov wrote:
>>>>> This is the right thing to do per se, but I am concerned that now
>>>>> people will just suffers from slow boot (it can take literally
>>>>> minutes) and will not realize the root cause nor that it's fixable
>>>>> (e.g. with rodata=n) and will probably just blame KASAN for slowness.
>>>>>
>>>>> Could we default this rodata check to n under KASAN? Or at least print
>>>>> some explanatory warning message before doing marking rodata (it
>>>>> should be printed right before "hang", so if you stare at it for a
>>>>> minute during each boot you realize that it may be related)? Or
>>>>> something along these lines. FWIW in my builds I just always disable
>>>>> the check.
>>>>
>>>> That certainly makes sense and we emit such warnings in other places
>>>> already (lockdep, trace_printk ...)
>>>>
>>>
>>> Agreed, but perhaps it would be better to make this code faster for KASAN=y?
>>> The main problem here is that we have many pgd entries containing kasan_zero_pud values
>>> and ptdump walker checks kasan_zero_pud many times.
>>> Instead, we could check it only once and skip further kasan_zero_pud's.
>>>
>>> I can't say I like this hack very much, but it wins me almost 20 seconds of boot time.
>>> Any objections?


Looks good to me.


>> Now I remember that we already discussed it in this thread:
>> https://lkml.org/lkml/2016/11/8/775
>>
>> Andrey, you proposed:
>>
>> "I didn't look at any code, but we probably could can remember last
>> visited pgd and skip next pgd if it's the same as previous."
>>
>> Do you still think it's a good idea?
>
> Ah, indeed. It will do roughly the same but with less of code churn, see bellow.
>
>> Walking the same pgd multiple times does not make sense (right?). And
>> it could probably speedup non-kasan builds to some degree in some
>> contexts. And the code will be free of additional ifdefs.
>>
>
> We could make it without ifdefs but this would be useless for KASAN=n
> as page table entries normally unique. So I'm thinking to add #ifdef
> at least for documentation purposes.
>
>
>
> diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
> index 8aa6bea..1599a5c 100644
> --- a/arch/x86/mm/dump_pagetables.c
> +++ b/arch/x86/mm/dump_pagetables.c
> @@ -373,6 +373,11 @@ static inline bool is_hypervisor_range(int idx)
>  #endif
>  }
>
> +static bool pgd_already_checked(pgd_t *prev_pgd, pgd_t *pgd, bool checkwx)
> +{
> +       return checkwx && prev_pgd && (pgd_val(*prev_pgd) == pgd_val(*pgd));
> +}
> +
>  static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
>                                        bool checkwx)
>  {
> @@ -381,6 +386,7 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
>  #else
>         pgd_t *start = swapper_pg_dir;
>  #endif
> +       pgd_t *prev_pgd = NULL;
>         pgprotval_t prot;
>         int i;
>         struct pg_state st = {};
> @@ -396,7 +402,8 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
>
>         for (i = 0; i < PTRS_PER_PGD; i++) {
>                 st.current_address = normalize_addr(i * PGD_LEVEL_MULT);
> -               if (!pgd_none(*start) && !is_hypervisor_range(i)) {
> +               if (!pgd_none(*start) && !is_hypervisor_range(i) &&
> +                               !pgd_already_checked(prev_pgd, start, checkwx)) {
>                         if (pgd_large(*start) || !pgd_present(*start)) {
>                                 prot = pgd_flags(*start);
>                                 note_page(m, &st, __pgprot(prot), 1);
> @@ -408,6 +415,7 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
>                         note_page(m, &st, __pgprot(0), 1);
>
>                 cond_resched();
> +               prev_pgd = start;
>                 start++;
>         }
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to kasan-dev@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/730837a1-ee6f-9891-0421-93616dd1c4eb%40virtuozzo.com.
> For more options, visit https://groups.google.com/d/optout.

[toc] | [prev] | [next] | [standalone]

#1578509

Hi,

On Fri, Feb 10, 2017 at 04:56:19PM +0300, Andrey Ryabinin wrote:
> On 02/10/2017 04:02 PM, Dmitry Vyukov wrote:
> > On Fri, Feb 10, 2017 at 1:15 PM, Andrey Ryabinin
> > <aryabinin@virtuozzo.com> wrote:
> >> On 02/10/2017 02:18 PM, Thomas Gleixner wrote:
> >>> On Fri, 10 Feb 2017, Dmitry Vyukov wrote:

> diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
> index 8aa6bea..1599a5c 100644
> --- a/arch/x86/mm/dump_pagetables.c
> +++ b/arch/x86/mm/dump_pagetables.c
> @@ -373,6 +373,11 @@ static inline bool is_hypervisor_range(int idx)
>  #endif
>  }
>  
> +static bool pgd_already_checked(pgd_t *prev_pgd, pgd_t *pgd, bool checkwx)
> +{
> +       return checkwx && prev_pgd && (pgd_val(*prev_pgd) == pgd_val(*pgd));
> +}
> +
>  static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
>                                        bool checkwx)
>  {
> @@ -381,6 +386,7 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
>  #else
>         pgd_t *start = swapper_pg_dir;
>  #endif
> +       pgd_t *prev_pgd = NULL;
>         pgprotval_t prot;
>         int i;
>         struct pg_state st = {};
> @@ -396,7 +402,8 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
>  
>         for (i = 0; i < PTRS_PER_PGD; i++) {
>                 st.current_address = normalize_addr(i * PGD_LEVEL_MULT);
> -               if (!pgd_none(*start) && !is_hypervisor_range(i)) {
> +               if (!pgd_none(*start) && !is_hypervisor_range(i) &&
> +                               !pgd_already_checked(prev_pgd, start, checkwx)) {

This means we'll fall into the else case...

>                         if (pgd_large(*start) || !pgd_present(*start)) {
>                                 prot = pgd_flags(*start);
>                                 note_page(m, &st, __pgprot(prot), 1);
> @@ -408,6 +415,7 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
>                         note_page(m, &st, __pgprot(0), 1);

... i.e. the note_page() here, where we'll claim that the's nothing
present due to the empty prot.

That'll give erroneous output for the userspace pagetable dumps, so I do
not think this is quite right, even though it gives a boot-time speedup.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1578520

On Fri, Feb 10, 2017 at 05:38:20PM +0300, Andrey Ryabinin wrote:
> On 02/10/2017 05:29 PM, Mark Rutland wrote:
> > On Fri, Feb 10, 2017 at 04:56:19PM +0300, Andrey Ryabinin wrote:
> >> On 02/10/2017 04:02 PM, Dmitry Vyukov wrote:
> >>> On Fri, Feb 10, 2017 at 1:15 PM, Andrey Ryabinin
> >>> <aryabinin@virtuozzo.com> wrote:
> >>>> On 02/10/2017 02:18 PM, Thomas Gleixner wrote:
> >>>>> On Fri, 10 Feb 2017, Dmitry Vyukov wrote:
> > 
> >> diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
> >> index 8aa6bea..1599a5c 100644
> >> --- a/arch/x86/mm/dump_pagetables.c
> >> +++ b/arch/x86/mm/dump_pagetables.c
> >> @@ -373,6 +373,11 @@ static inline bool is_hypervisor_range(int idx)
> >>  #endif
> >>  }
> >>  
> >> +static bool pgd_already_checked(pgd_t *prev_pgd, pgd_t *pgd, bool checkwx)
> >> +{
> >> +       return checkwx && prev_pgd && (pgd_val(*prev_pgd) == pgd_val(*pgd));
> >> +}
> >> +
> >>  static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
> >>                                        bool checkwx)
> >>  {
> >> @@ -381,6 +386,7 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
> >>  #else
> >>         pgd_t *start = swapper_pg_dir;
> >>  #endif
> >> +       pgd_t *prev_pgd = NULL;
> >>         pgprotval_t prot;
> >>         int i;
> >>         struct pg_state st = {};
> >> @@ -396,7 +402,8 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
> >>  
> >>         for (i = 0; i < PTRS_PER_PGD; i++) {
> >>                 st.current_address = normalize_addr(i * PGD_LEVEL_MULT);
> >> -               if (!pgd_none(*start) && !is_hypervisor_range(i)) {
> >> +               if (!pgd_none(*start) && !is_hypervisor_range(i) &&
> >> +                               !pgd_already_checked(prev_pgd, start, checkwx)) {
> > 
> > This means we'll fall into the else case...
> > 
> >>                         if (pgd_large(*start) || !pgd_present(*start)) {
> >>                                 prot = pgd_flags(*start);
> >>                                 note_page(m, &st, __pgprot(prot), 1);
> >> @@ -408,6 +415,7 @@ static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
> >>                         note_page(m, &st, __pgprot(0), 1);
> > 
> > ... i.e. the note_page() here, where we'll claim that the's nothing
> > present due to the empty prot.
> > 
> > That'll give erroneous output for the userspace pagetable dumps, so I do
> > not think this is quite right, even though it gives a boot-time speedup.
> > 
> 
> For userspace pagetable dumps checkwx is false, so
> page_already_checked() will return false and will not go into else.
> userspace pagetable dumps works as before.

Ah. I missed that; sorry for the noise.

That sounds ok then, though it's probably worth a comment as to what
we're doing this for.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1578530

On Fri, Feb 10, 2017 at 02:02:19PM +0100, Dmitry Vyukov wrote:
> On Fri, Feb 10, 2017 at 1:15 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
> >
> >
> > On 02/10/2017 02:18 PM, Thomas Gleixner wrote:
> >> On Fri, 10 Feb 2017, Dmitry Vyukov wrote:

> >>> Could we default this rodata check to n under KASAN? Or at least print
> >>> some explanatory warning message before doing marking rodata (it
> >>> should be printed right before "hang", so if you stare at it for a
> >>> minute during each boot you realize that it may be related)?

Regardless of any optimisation work, I think it would make sense to both
log something in the debug_check_wx() and to default it off if KASAN is
selected.

That could be generic, so we don't have to alter each and every arch
port...

The WX check is a one-time boot check, and missing it for a KASAN kernel
isn't the end of the world, though I'd still like to be able to use the
userspace page table dumps.

Thanks,
Mark.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web