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

Groups > linux.kernel > #1448171 > unrolled thread

[PATCH 00/19] x86/dumpstack: rewrite x86 stack dump code

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/19] x86/dumpstack: rewrite x86 stack dump code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
    [PATCH 05/19] x86/dumpstack: fix function graph tracing stack dump reliability issues Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
    [PATCH 04/19] x86/dumpstack: make printk_stack_address() more generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
    [PATCH 18/19] x86/dumpstack: print stack identifier on its own line Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
    [PATCH 13/19] x86/stacktrace: convert save_stack_trace_*() to the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
    [PATCH 19/19] x86/dumpstack: print any pt_regs found on the stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
      Re: [PATCH 19/19] x86/dumpstack: print any pt_regs found on the stack Andy Lutomirski <luto@amacapital.net> - 2016-07-22 00:40 +0200
        Re: [PATCH 19/19] x86/dumpstack: print any pt_regs found on the stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-22 05:40 +0200
          Re: [PATCH 19/19] x86/dumpstack: print any pt_regs found on the stack Andy Lutomirski <luto@amacapital.net> - 2016-07-22 07:20 +0200
            Re: [PATCH 19/19] x86/dumpstack: print any pt_regs found on the stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-22 18:00 +0200
              Re: [PATCH 19/19] x86/dumpstack: print any pt_regs found on the stack Andy Lutomirski <luto@amacapital.net> - 2016-07-22 23:50 +0200
                Re: [PATCH 19/19] x86/dumpstack: print any pt_regs found on the stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-23 00:30 +0200
                  Re: [PATCH 19/19] x86/dumpstack: print any pt_regs found on the stack Andy Lutomirski <luto@amacapital.net> - 2016-07-23 01:20 +0200
                    Re: [PATCH 19/19] x86/dumpstack: print any pt_regs found on the stack Andy Lutomirski <luto@amacapital.net> - 2016-07-23 01:40 +0200
                      Re: [PATCH 19/19] x86/dumpstack: print any pt_regs found on the stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-23 02:10 +0200
                    Re: [PATCH 19/19] x86/dumpstack: print any pt_regs found on the stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-23 01:40 +0200
    [PATCH 08/19] x86/dumpstack: don't disable preemption in show_stack_log_lvl() and dump_trace() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
    [PATCH 14/19] oprofile/x86: convert x86_backtrace() to the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
    [PATCH 06/19] x86/dumpstack: remove extra brackets around "EOE" Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
    [PATCH 10/19] x86/dumpstack: add get_stack_info() interface Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
      Re: [PATCH 10/19] x86/dumpstack: add get_stack_info() interface Andy Lutomirski <luto@amacapital.net> - 2016-07-23 01:30 +0200
        Re: [PATCH 10/19] x86/dumpstack: add get_stack_info() interface Andy Lutomirski <luto@amacapital.net> - 2016-07-23 02:00 +0200
          Re: [PATCH 10/19] x86/dumpstack: add get_stack_info() interface Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-23 15:10 +0200
        Re: [PATCH 10/19] x86/dumpstack: add get_stack_info() interface Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-23 02:00 +0200
          Re: [PATCH 10/19] x86/dumpstack: add get_stack_info() interface Andy Lutomirski <luto@amacapital.net> - 2016-07-23 02:20 +0200
            Re: [PATCH 10/19] x86/dumpstack: add get_stack_info() interface Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-23 16:10 +0200
    [PATCH 01/19] x86/dumpstack: remove show_trace() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
      Re: [PATCH 01/19] x86/dumpstack: remove show_trace() Andy Lutomirski <luto@amacapital.net> - 2016-07-22 00:00 +0200
    [PATCH 12/19] perf/x86: convert perf_callchain_kernel() to the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
    [PATCH 02/19] x86/dumpstack: add get_stack_pointer() and get_frame_pointer() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
      Re: [PATCH 02/19] x86/dumpstack: add get_stack_pointer() and get_frame_pointer() Andy Lutomirski <luto@amacapital.net> - 2016-07-22 00:00 +0200
    [PATCH 11/19] x86/dumptrace: add new unwind interface and implementations Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
    [PATCH 09/19] x86/dumpstack: simplify in_exception_stack() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
      Re: [PATCH 09/19] x86/dumpstack: simplify in_exception_stack() Andy Lutomirski <luto@amacapital.net> - 2016-07-22 00:10 +0200
    [PATCH 07/19] x86/dumpstack: add IRQ_USABLE_STACK_SIZE define Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-21 23:30 +0200
      Re: [PATCH 07/19] x86/dumpstack: add IRQ_USABLE_STACK_SIZE define Andy Lutomirski <luto@amacapital.net> - 2016-07-22 00:10 +0200
        Re: [PATCH 07/19] x86/dumpstack: add IRQ_USABLE_STACK_SIZE define Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-22 03:50 +0200
          Re: [PATCH 07/19] x86/dumpstack: add IRQ_USABLE_STACK_SIZE define Ingo Molnar <mingo@kernel.org> - 2016-07-22 10:30 +0200
    Re: [PATCH 00/19] x86/dumpstack: rewrite x86 stack dump code Linus Torvalds <torvalds@linux-foundation.org> - 2016-07-23 02:30 +0200
      Re: [PATCH 00/19] x86/dumpstack: rewrite x86 stack dump code Andy Lutomirski <luto@amacapital.net> - 2016-07-23 02:40 +0200
        Re: [PATCH 00/19] x86/dumpstack: rewrite x86 stack dump code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-23 07:40 +0200
          Re: [PATCH 00/19] x86/dumpstack: rewrite x86 stack dump code Linus Torvalds <torvalds@linux-foundation.org> - 2016-07-23 07:50 +0200
            Re: [PATCH 00/19] x86/dumpstack: rewrite x86 stack dump code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-07-23 15:00 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1448823 — Re: [PATCH 10/19] x86/dumpstack: add get_stack_info() interface

On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> valid_stack_ptr() is buggy: it assumes that all stacks are of size
> THREAD_SIZE, which is not true for exception stacks.  So the
> walk_stack() callbacks will need to know the location of the beginning
> of the stack as well as the end.
>
> Another issue is that in general the various features of a stack (type,
> size, next stack pointer, description string) are scattered around in
> various places throughout the stack dump code.

I finally figured out what visit_info is.  But would it make more
sense to track it in the unwind state so that the unwinder can
directly make sure it doesn't start looping?

And please remove test_and_set_bit() -- it's pointlessly slow.

> +static bool in_hardirq_stack(unsigned long *stack, struct stack_info *info,
> +                            unsigned long *visit_mask)
> +{
> +       unsigned long *begin = (unsigned long *)this_cpu_read(hardirq_stack);
> +       unsigned long *end   = begin + (THREAD_SIZE / sizeof(long));
> +
> +       if (stack < begin || stack >= end)
> +               return false;
> +
> +       if (visit_mask && test_and_set_bit(STACK_TYPE_IRQ, visit_mask))
> +               return false;
> +
> +       info->type      = STACK_TYPE_IRQ;
> +       info->begin     = begin;
> +       info->end       = end;
> +       info->next      = (unsigned long *)*begin;

This works, but it's a bit magic.  I don't suppose we could get rid of
this ->next thing entirely and teach show_stack_log_lvl(), etc. to
move from stack to stack by querying the stack type of whatever the
frame base address is if the frame base address ends up being out of
bounds for the current stack?  Or maybe the unwinder could even do
this by itself.

> +static bool in_exception_stack(unsigned long *s, struct stack_info *info,
> +                              unsigned long *visit_mask)
>  {
>         unsigned long stack = (unsigned long)s;
>         unsigned long begin, end;
> @@ -44,55 +63,62 @@ static unsigned long *in_exception_stack(unsigned long *s, char **name,
>                 if (stack < begin || stack >= end)
>                         continue;
>
> -               if (test_and_set_bit(k, visit_mask))
> +               if (visit_mask &&
> +                   test_and_set_bit(STACK_TYPE_EXCEPTION + k, visit_mask))
>                         return false;
>
> -               *name = exception_stack_names[k];
> -               return (unsigned long *)end;
> +               info->type      = STACK_TYPE_EXCEPTION + k;
> +               info->begin     = (unsigned long *)begin;
> +               info->end       = (unsigned long *)end;
> +               info->next      = (unsigned long *)info->end[-2];

This is so magical that I don't immediately see why it's correct.
Presumably it's because the thing two slots down from the top of the
stack is regs->sp?  If so, that needs a comment.

But again, couldn't we use the fact that we now know how to decode
pt_regs to avoid needing this?  I can imagine it being useful as a
fallback in the event that the unwinder fails, but this is just a
fallback.  Also, NMI is weird and I'm wondering whether this works at
all when trying to unwind from a looped NMI.

Fixing this up could be a followup after this series is in, I think --
you're preserving existing behavior AFAICS.  I just don't particularly
like the existing behavior.

FWIW, I think this code needs to be explicitly tested for the 32-bit
double fault case.  It's highly magical.

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

#1448840 — Re: [PATCH 10/19] x86/dumpstack: add get_stack_info() interface

On Fri, Jul 22, 2016 at 4:26 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> valid_stack_ptr() is buggy: it assumes that all stacks are of size
>> THREAD_SIZE, which is not true for exception stacks.  So the
>> walk_stack() callbacks will need to know the location of the beginning
>> of the stack as well as the end.
>>
>> Another issue is that in general the various features of a stack (type,
>> size, next stack pointer, description string) are scattered around in
>> various places throughout the stack dump code.
>
> I finally figured out what visit_info is.  But would it make more
> sense to track it in the unwind state so that the unwinder can
> directly make sure it doesn't start looping?
>

I just realized that it *is* in the unwind state.  But maybe this code
in update_stack_state:

    sp = info->next;
    if (!sp).
        goto unknown;

    if (get_stack_info(sp, state->task, info, &state->stack_mask))
        goto unknown;

    if (!on_stack(info, addr, len))
        goto unknown;

should do something like:

if (get_stack_info(addr, ...))
  goto unknown.

sp = info->end;

instead.  Alternatively, maybe it would make sense to keep sp as is
(have update_stack_state return bool instead of returning a pointer)
so that a frame that switches stacks still shows the actual sp at the
time that the frame called whatever the it called.

I'm really quite confused by what state->sp means in your current
code.  In the non-stack-switching case (everything is on the thread
stack), it appears to always match state->bp.  Am I missing something?
 If I'm understanding this correctly, when you're pointing at a call
frame, state->bp is that frame's base address (the top of the stack
frame), unwind_get_return_address() returns the address to which that
frame would return, and, in the future, unwind_get_gpr(UNWIND_DI) or
whatever it ends up looking like will return RDI at the time that the
frame called whatever function it called, if known.  By that logic,
shouldn't state->sp be sp on entry to the call instruction?  (Or could
sp just be removed?  Does it do anything?)

I guess the reason I'm still not 100% comfortable with the idea that
pt_regs frames don't exist a real frames is that I keep waffling as to
how I should think about the regs associated with a frame in the
future DWARF world.  I think I imagine them being the regs at the time
that the frame did it's call to the next frame, which, by an
admittedly weak analogy, means that the pt_regs state would be the
regs at the time that the exception or interrupt happened.  That
offers a third silly option for dealing with the annoying '?': emit
two frames for a pt_regs, but have the frame in the entry code show
NULL for its return address because it's not a normal return.

> And please remove test_and_set_bit() -- it's pointlessly slow.
>
>> +static bool in_hardirq_stack(unsigned long *stack, struct stack_info *info,
>> +                            unsigned long *visit_mask)
>> +{
>> +       unsigned long *begin = (unsigned long *)this_cpu_read(hardirq_stack);
>> +       unsigned long *end   = begin + (THREAD_SIZE / sizeof(long));
>> +
>> +       if (stack < begin || stack >= end)
>> +               return false;
>> +
>> +       if (visit_mask && test_and_set_bit(STACK_TYPE_IRQ, visit_mask))
>> +               return false;
>> +
>> +       info->type      = STACK_TYPE_IRQ;
>> +       info->begin     = begin;
>> +       info->end       = end;
>> +       info->next      = (unsigned long *)*begin;
>
> This works, but it's a bit magic.  I don't suppose we could get rid of
> this ->next thing entirely and teach show_stack_log_lvl(), etc. to
> move from stack to stack by querying the stack type of whatever the
> frame base address is if the frame base address ends up being out of
> bounds for the current stack?  Or maybe the unwinder could even do
> this by itself.
>
>> +static bool in_exception_stack(unsigned long *s, struct stack_info *info,
>> +                              unsigned long *visit_mask)
>>  {
>>         unsigned long stack = (unsigned long)s;
>>         unsigned long begin, end;
>> @@ -44,55 +63,62 @@ static unsigned long *in_exception_stack(unsigned long *s, char **name,
>>                 if (stack < begin || stack >= end)
>>                         continue;
>>
>> -               if (test_and_set_bit(k, visit_mask))
>> +               if (visit_mask &&
>> +                   test_and_set_bit(STACK_TYPE_EXCEPTION + k, visit_mask))
>>                         return false;
>>
>> -               *name = exception_stack_names[k];
>> -               return (unsigned long *)end;
>> +               info->type      = STACK_TYPE_EXCEPTION + k;
>> +               info->begin     = (unsigned long *)begin;
>> +               info->end       = (unsigned long *)end;
>> +               info->next      = (unsigned long *)info->end[-2];
>
> This is so magical that I don't immediately see why it's correct.
> Presumably it's because the thing two slots down from the top of the
> stack is regs->sp?  If so, that needs a comment.
>
> But again, couldn't we use the fact that we now know how to decode
> pt_regs to avoid needing this?  I can imagine it being useful as a
> fallback in the event that the unwinder fails, but this is just a
> fallback.  Also, NMI is weird and I'm wondering whether this works at
> all when trying to unwind from a looped NMI.
>
> Fixing this up could be a followup after this series is in, I think --
> you're preserving existing behavior AFAICS.  I just don't particularly
> like the existing behavior.
>
> FWIW, I think this code needs to be explicitly tested for the 32-bit
> double fault case.  It's highly magical.



-- 
Andy Lutomirski
AMA Capital Management, LLC

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

#1448945 — Re: [PATCH 10/19] x86/dumpstack: add get_stack_info() interface

On Fri, Jul 22, 2016 at 04:52:10PM -0700, Andy Lutomirski wrote:
> On Fri, Jul 22, 2016 at 4:26 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> > On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> valid_stack_ptr() is buggy: it assumes that all stacks are of size
> >> THREAD_SIZE, which is not true for exception stacks.  So the
> >> walk_stack() callbacks will need to know the location of the beginning
> >> of the stack as well as the end.
> >>
> >> Another issue is that in general the various features of a stack (type,
> >> size, next stack pointer, description string) are scattered around in
> >> various places throughout the stack dump code.
> >
> > I finally figured out what visit_info is.  But would it make more
> > sense to track it in the unwind state so that the unwinder can
> > directly make sure it doesn't start looping?
> >
> 
> I just realized that it *is* in the unwind state.  But maybe this code
> in update_stack_state:
> 
>     sp = info->next;
>     if (!sp).
>         goto unknown;
> 
>     if (get_stack_info(sp, state->task, info, &state->stack_mask))
>         goto unknown;
> 
>     if (!on_stack(info, addr, len))
>         goto unknown;
> 
> should do something like:
> 
> if (get_stack_info(addr, ...))
>   goto unknown.
> 
> sp = info->end;
> 
> instead.  Alternatively, maybe it would make sense to keep sp as is
> (have update_stack_state return bool instead of returning a pointer)
> so that a frame that switches stacks still shows the actual sp at the
> time that the frame called whatever the it called.
> 
> I'm really quite confused by what state->sp means in your current
> code.  In the non-stack-switching case (everything is on the thread
> stack), it appears to always match state->bp.  Am I missing something?
>  If I'm understanding this correctly, when you're pointing at a call
> frame, state->bp is that frame's base address (the top of the stack
> frame), unwind_get_return_address() returns the address to which that
> frame would return, and, in the future, unwind_get_gpr(UNWIND_DI) or
> whatever it ends up looking like will return RDI at the time that the
> frame called whatever function it called, if known.  By that logic,
> shouldn't state->sp be sp on entry to the call instruction?  (Or could
> sp just be removed?  Does it do anything?)

Yeah, I think sp has no purpose and can actually just be removed.

(It was leftover from a previous iteration of the code where it did have
a purpose and I forgot to remove it.)

> I guess the reason I'm still not 100% comfortable with the idea that
> pt_regs frames don't exist a real frames is that I keep waffling as to
> how I should think about the regs associated with a frame in the
> future DWARF world.  I think I imagine them being the regs at the time
> that the frame did it's call to the next frame, which, by an
> admittedly weak analogy, means that the pt_regs state would be the
> regs at the time that the exception or interrupt happened.  That
> offers a third silly option for dealing with the annoying '?': emit
> two frames for a pt_regs, but have the frame in the entry code show
> NULL for its return address because it's not a normal return.

Well, I'd say let's not get ahead of ourselves.  I think the current
regs-aren't-a-frame design works fine for now, and the code is fairly
simple.  If/when we get a DWARF unwinder, we can revisit that decision.

-- 
Josh

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

#1448841 — Re: [PATCH 10/19] x86/dumpstack: add get_stack_info() interface

On Fri, Jul 22, 2016 at 04:26:46PM -0700, Andy Lutomirski wrote:
> On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > valid_stack_ptr() is buggy: it assumes that all stacks are of size
> > THREAD_SIZE, which is not true for exception stacks.  So the
> > walk_stack() callbacks will need to know the location of the beginning
> > of the stack as well as the end.
> >
> > Another issue is that in general the various features of a stack (type,
> > size, next stack pointer, description string) are scattered around in
> > various places throughout the stack dump code.
> 
> I finally figured out what visit_info is.  But would it make more
> sense to track it in the unwind state so that the unwinder can
> directly make sure it doesn't start looping?

Well, the unwinders aren't the only users of get_stack_info() and the
visit_mask.  show_trace_log_lvl() also uses it.

But it would probably be cleaner to at least do the visit_mask bit
testing/setting in get_stack_info() rather than in the in_*_stack()
functions.

> And please remove test_and_set_bit() -- it's pointlessly slow.

Ok.

> 
> > +static bool in_hardirq_stack(unsigned long *stack, struct stack_info *info,
> > +                            unsigned long *visit_mask)
> > +{
> > +       unsigned long *begin = (unsigned long *)this_cpu_read(hardirq_stack);
> > +       unsigned long *end   = begin + (THREAD_SIZE / sizeof(long));
> > +
> > +       if (stack < begin || stack >= end)
> > +               return false;
> > +
> > +       if (visit_mask && test_and_set_bit(STACK_TYPE_IRQ, visit_mask))
> > +               return false;
> > +
> > +       info->type      = STACK_TYPE_IRQ;
> > +       info->begin     = begin;
> > +       info->end       = end;
> > +       info->next      = (unsigned long *)*begin;
> 
> This works, but it's a bit magic.  I don't suppose we could get rid of
> this ->next thing entirely and teach show_stack_log_lvl(), etc. to
> move from stack to stack by querying the stack type of whatever the
> frame base address is if the frame base address ends up being out of
> bounds for the current stack?  Or maybe the unwinder could even do
> this by itself.

I'm not quite sure what you mean here.  The ->next stack pointer is
quite useful and it abstracts that ugliness away from the callers of
get_stack_info().  I'm open to any specific suggestions.

> 
> > +static bool in_exception_stack(unsigned long *s, struct stack_info *info,
> > +                              unsigned long *visit_mask)
> >  {
> >         unsigned long stack = (unsigned long)s;
> >         unsigned long begin, end;
> > @@ -44,55 +63,62 @@ static unsigned long *in_exception_stack(unsigned long *s, char **name,
> >                 if (stack < begin || stack >= end)
> >                         continue;
> >
> > -               if (test_and_set_bit(k, visit_mask))
> > +               if (visit_mask &&
> > +                   test_and_set_bit(STACK_TYPE_EXCEPTION + k, visit_mask))
> >                         return false;
> >
> > -               *name = exception_stack_names[k];
> > -               return (unsigned long *)end;
> > +               info->type      = STACK_TYPE_EXCEPTION + k;
> > +               info->begin     = (unsigned long *)begin;
> > +               info->end       = (unsigned long *)end;
> > +               info->next      = (unsigned long *)info->end[-2];
> 
> This is so magical that I don't immediately see why it's correct.
> Presumably it's because the thing two slots down from the top of the
> stack is regs->sp?  If so, that needs a comment.

Heck if I know, I just stole it from dump_trace() ;-)

I'll figure it out and add a comment.

> But again, couldn't we use the fact that we now know how to decode
> pt_regs to avoid needing this?  I can imagine it being useful as a
> fallback in the event that the unwinder fails, but this is just a
> fallback.

Yeah, this is needed as a fallback.  But I wouldn't call it "just" a
fallback: the stack dump code *needs* to be able to still traverse the
stacks if frame pointers fail.

> Also, NMI is weird and I'm wondering whether this works at
> all when trying to unwind from a looped NMI.

Unless I'm missing something, I think it should be fine for nested NMIs,
since they're all on the same stack.  I can try to test it.  What in
particular are you worried about?

> Fixing this up could be a followup after this series is in, I think --
> you're preserving existing behavior AFAICS.  I just don't particularly
> like the existing behavior.
> 
> FWIW, I think this code needs to be explicitly tested for the 32-bit
> double fault case.  It's highly magical.

Ok, I'll test it.

-- 
Josh

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

#1448847 — Re: [PATCH 10/19] x86/dumpstack: add get_stack_info() interface

On Fri, Jul 22, 2016 at 4:54 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > +static bool in_hardirq_stack(unsigned long *stack, struct stack_info *info,
>> > +                            unsigned long *visit_mask)
>> > +{
>> > +       unsigned long *begin = (unsigned long *)this_cpu_read(hardirq_stack);
>> > +       unsigned long *end   = begin + (THREAD_SIZE / sizeof(long));
>> > +
>> > +       if (stack < begin || stack >= end)
>> > +               return false;
>> > +
>> > +       if (visit_mask && test_and_set_bit(STACK_TYPE_IRQ, visit_mask))
>> > +               return false;
>> > +
>> > +       info->type      = STACK_TYPE_IRQ;
>> > +       info->begin     = begin;
>> > +       info->end       = end;
>> > +       info->next      = (unsigned long *)*begin;
>>
>> This works, but it's a bit magic.  I don't suppose we could get rid of
>> this ->next thing entirely and teach show_stack_log_lvl(), etc. to
>> move from stack to stack by querying the stack type of whatever the
>> frame base address is if the frame base address ends up being out of
>> bounds for the current stack?  Or maybe the unwinder could even do
>> this by itself.
>
> I'm not quite sure what you mean here.  The ->next stack pointer is
> quite useful and it abstracts that ugliness away from the callers of
> get_stack_info().  I'm open to any specific suggestions.

So far I've found two users of this thing.  One is
show_stack_log_lvl(), and it makes sense there, but maybe
info->heuristic_next_stack would be a better name.  The other is the
unwinder itself, and I think that walking from stack to stack using
this heuristic is the wrong approach there, at least in the long term.
I'd rather we just followed the bp chain wherever it leads us, as long
as it leads us to a valid stack that we haven't visited before.

As a concrete example of what I think is wrong with the current
approach, ISTM it would be totally valid to implement stack switching
like this:

some_func:
 push %rbp
 mov %rsp, %rbp
 ...
 mov [next stack], %rsp
 call some_other_func
 mov %rbp, %rsp
 pop %rbp
 ret

With the current approach, you can't unwind out of that function,
because there's no way to populate info->next.  I'm not actually
suggesting that the kernel should ever do such a thing on x86, and my
proposed rewrite of the IRQ stack code [1] will be fully compatible
with your approach, but it seems odd to me that the unwinder should
depend on idea that the stacks in use are chained together in a way
that can be decoded without .  (But maybe some of the Go compilers do
work this way -- I've never looked at their precise stack layout.)

Also, if you ever intend to port this thing to other architectures, I
think there are architectures that have separate exception stacks and
that track the next available slot on those stacks dynamically.  I
think that x86_32 is an example of this if task gates are used in a
back-and-forth manner, although Linux doesn't do that.  (x86_64 should
have done this for IST, but it didn't.)  On those architectures, you
can have two separate switches onto the same stack live at the same
time, and your current approach won't work.  (Even if you make the
change I'm suggesting, visit_mask will break, too, but fixing that
would be a much less invasive change.)

Am I making any sense?  This is a suggestion for making it better, not
something I see as a requirement for getting the x86 code upstream.

>>
>> > +static bool in_exception_stack(unsigned long *s, struct stack_info *info,
>> > +                              unsigned long *visit_mask)
>> >  {
>> >         unsigned long stack = (unsigned long)s;
>> >         unsigned long begin, end;
>> > @@ -44,55 +63,62 @@ static unsigned long *in_exception_stack(unsigned long *s, char **name,
>> >                 if (stack < begin || stack >= end)
>> >                         continue;
>> >
>> > -               if (test_and_set_bit(k, visit_mask))
>> > +               if (visit_mask &&
>> > +                   test_and_set_bit(STACK_TYPE_EXCEPTION + k, visit_mask))
>> >                         return false;
>> >
>> > -               *name = exception_stack_names[k];
>> > -               return (unsigned long *)end;
>> > +               info->type      = STACK_TYPE_EXCEPTION + k;
>> > +               info->begin     = (unsigned long *)begin;
>> > +               info->end       = (unsigned long *)end;
>> > +               info->next      = (unsigned long *)info->end[-2];
>>
>> This is so magical that I don't immediately see why it's correct.
>> Presumably it's because the thing two slots down from the top of the
>> stack is regs->sp?  If so, that needs a comment.
>
> Heck if I know, I just stole it from dump_trace() ;-)
>
> I'll figure it out and add a comment.

If you can write it as:

struct pt_regs *regs = (struct pt_regs *)end - 1;
info->next = regs->sp;

and it still works, then no comment required :)

>
>> But again, couldn't we use the fact that we now know how to decode
>> pt_regs to avoid needing this?  I can imagine it being useful as a
>> fallback in the event that the unwinder fails, but this is just a
>> fallback.
>
> Yeah, this is needed as a fallback.  But I wouldn't call it "just" a
> fallback: the stack dump code *needs* to be able to still traverse the
> stacks if frame pointers fail.
>
>> Also, NMI is weird and I'm wondering whether this works at
>> all when trying to unwind from a looped NMI.
>
> Unless I'm missing something, I think it should be fine for nested NMIs,
> since they're all on the same stack.  I can try to test it.  What in
> particular are you worried about?
>

The top of the NMI frame contains no less than *three* (SS, SP, FLAGS,
CS, IP) records.  Off the top of my head, the record that matters is
the third one, so it should be regs[-15].  If an MCE hits while this
mess is being set up, good luck unwinding *that*.  If you really want
to know, take a deep breath, read the long comment in entry_64.S after
.Lnmi_from_kernel, then give up on x86 and start hacking on some other
architecture.

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

#1448950 — Re: [PATCH 10/19] x86/dumpstack: add get_stack_info() interface

On Fri, Jul 22, 2016 at 05:15:03PM -0700, Andy Lutomirski wrote:
> On Fri, Jul 22, 2016 at 4:54 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > +static bool in_hardirq_stack(unsigned long *stack, struct stack_info *info,
> >> > +                            unsigned long *visit_mask)
> >> > +{
> >> > +       unsigned long *begin = (unsigned long *)this_cpu_read(hardirq_stack);
> >> > +       unsigned long *end   = begin + (THREAD_SIZE / sizeof(long));
> >> > +
> >> > +       if (stack < begin || stack >= end)
> >> > +               return false;
> >> > +
> >> > +       if (visit_mask && test_and_set_bit(STACK_TYPE_IRQ, visit_mask))
> >> > +               return false;
> >> > +
> >> > +       info->type      = STACK_TYPE_IRQ;
> >> > +       info->begin     = begin;
> >> > +       info->end       = end;
> >> > +       info->next      = (unsigned long *)*begin;
> >>
> >> This works, but it's a bit magic.  I don't suppose we could get rid of
> >> this ->next thing entirely and teach show_stack_log_lvl(), etc. to
> >> move from stack to stack by querying the stack type of whatever the
> >> frame base address is if the frame base address ends up being out of
> >> bounds for the current stack?  Or maybe the unwinder could even do
> >> this by itself.
> >
> > I'm not quite sure what you mean here.  The ->next stack pointer is
> > quite useful and it abstracts that ugliness away from the callers of
> > get_stack_info().  I'm open to any specific suggestions.
> 
> So far I've found two users of this thing.  One is
> show_stack_log_lvl(), and it makes sense there, but maybe
> info->heuristic_next_stack would be a better name.  The other is the
> unwinder itself, and I think that walking from stack to stack using
> this heuristic is the wrong approach there, at least in the long term.
> I'd rather we just followed the bp chain wherever it leads us, as long
> as it leads us to a valid stack that we haven't visited before.
>
> As a concrete example of what I think is wrong with the current
> approach, ISTM it would be totally valid to implement stack switching
> like this:
> 
> some_func:
>  push %rbp
>  mov %rsp, %rbp
>  ...
>  mov [next stack], %rsp
>  call some_other_func
>  mov %rbp, %rsp
>  pop %rbp
>  ret
> 
> With the current approach, you can't unwind out of that function,
> because there's no way to populate info->next.  I'm not actually
> suggesting that the kernel should ever do such a thing on x86, and my
> proposed rewrite of the IRQ stack code [1] will be fully compatible
> with your approach, but it seems odd to me that the unwinder should
> depend on idea that the stacks in use are chained together in a way
> that can be decoded without .  (But maybe some of the Go compilers do
> work this way -- I've never looked at their precise stack layout.)

I don't think relying on frame pointers to switch between stacks is
necessarily a good idea:

- It requires CONFIG_FRAME_POINTER, which makes it unwinder-specific.
  The current approach is unwinder-agnostic.

- Instead of relying on a single correct "next stack" pointer, it
  requires relying on potentially dozens of correct frame pointers,
  across multiple stacks.  So a lot of things have to go right, instead
  of just one.  And then show_trace_log_lvl() becomes more dependent on
  the unwinder not screwing things up.

> Also, if you ever intend to port this thing to other architectures, I
> think there are architectures that have separate exception stacks and
> that track the next available slot on those stacks dynamically.  I
> think that x86_32 is an example of this if task gates are used in a
> back-and-forth manner, although Linux doesn't do that.  (x86_64 should
> have done this for IST, but it didn't.)  On those architectures, you
> can have two separate switches onto the same stack live at the same
> time, and your current approach won't work.  (Even if you make the
> change I'm suggesting, visit_mask will break, too, but fixing that
> would be a much less invasive change.)
>
> Am I making any sense?  This is a suggestion for making it better, not
> something I see as a requirement for getting the x86 code upstream.

I think porting these interfaces to other architectures could eventually
be a good idea, and you're right that the current approach might need to
be tweaked in order to work everywhere.  (But I agree this needs more
thought and this discussion can wait until later.)

> >> > +static bool in_exception_stack(unsigned long *s, struct stack_info *info,
> >> > +                              unsigned long *visit_mask)
> >> >  {
> >> >         unsigned long stack = (unsigned long)s;
> >> >         unsigned long begin, end;
> >> > @@ -44,55 +63,62 @@ static unsigned long *in_exception_stack(unsigned long *s, char **name,
> >> >                 if (stack < begin || stack >= end)
> >> >                         continue;
> >> >
> >> > -               if (test_and_set_bit(k, visit_mask))
> >> > +               if (visit_mask &&
> >> > +                   test_and_set_bit(STACK_TYPE_EXCEPTION + k, visit_mask))
> >> >                         return false;
> >> >
> >> > -               *name = exception_stack_names[k];
> >> > -               return (unsigned long *)end;
> >> > +               info->type      = STACK_TYPE_EXCEPTION + k;
> >> > +               info->begin     = (unsigned long *)begin;
> >> > +               info->end       = (unsigned long *)end;
> >> > +               info->next      = (unsigned long *)info->end[-2];
> >>
> >> This is so magical that I don't immediately see why it's correct.
> >> Presumably it's because the thing two slots down from the top of the
> >> stack is regs->sp?  If so, that needs a comment.
> >
> > Heck if I know, I just stole it from dump_trace() ;-)
> >
> > I'll figure it out and add a comment.
> 
> If you can write it as:
> 
> struct pt_regs *regs = (struct pt_regs *)end - 1;
> info->next = regs->sp;
> 
> and it still works, then no comment required :)

Yeah.  in_irq_stack() does something similar, though it uses end[-1].
And its regs are actually stored on the thread stack.  So something
doesn't quite add up for irqs.  I still need to do some homework there.

> >> But again, couldn't we use the fact that we now know how to decode
> >> pt_regs to avoid needing this?  I can imagine it being useful as a
> >> fallback in the event that the unwinder fails, but this is just a
> >> fallback.
> >
> > Yeah, this is needed as a fallback.  But I wouldn't call it "just" a
> > fallback: the stack dump code *needs* to be able to still traverse the
> > stacks if frame pointers fail.
> >
> >> Also, NMI is weird and I'm wondering whether this works at
> >> all when trying to unwind from a looped NMI.
> >
> > Unless I'm missing something, I think it should be fine for nested NMIs,
> > since they're all on the same stack.  I can try to test it.  What in
> > particular are you worried about?
> >
> 
> The top of the NMI frame contains no less than *three* (SS, SP, FLAGS,
> CS, IP) records.  Off the top of my head, the record that matters is
> the third one, so it should be regs[-15].  If an MCE hits while this
> mess is being set up, good luck unwinding *that*.  If you really want
> to know, take a deep breath, read the long comment in entry_64.S after
> .Lnmi_from_kernel, then give up on x86 and start hacking on some other
> architecture.

I did read that comment.  Fortunately there's a big difference between
reading and understanding so I can go on being an ignorant x86 hacker!

For nested NMIs, it does look like the stack of the exception which
interrupted the first NMI would get skipped by the stack dump.  (But
that's a general problem, not specific to my patch set.)

Am I correct in understanding that there can only be one level of NMI
nesting at any given time?  If so, could we make it easier on the
unwinder by putting the nested NMI on a separate software stack, so the
"next stack" pointers are always in the same place?  Or am I just being
naive?

-- 
Josh

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

#1448184 — [PATCH 01/19] x86/dumpstack: remove show_trace()

There are a bewildering array of options for dumping the stack.
Simplify things a little by removing show_trace(), which is unused.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/kdebug.h | 2 --
 arch/x86/kernel/dumpstack.c   | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/arch/x86/include/asm/kdebug.h b/arch/x86/include/asm/kdebug.h
index 1ef9d58..d318811 100644
--- a/arch/x86/include/asm/kdebug.h
+++ b/arch/x86/include/asm/kdebug.h
@@ -24,8 +24,6 @@ enum die_val {
 extern void printk_address(unsigned long address);
 extern void die(const char *, struct pt_regs *,long);
 extern int __must_check __die(const char *, struct pt_regs *, long);
-extern void show_trace(struct task_struct *t, struct pt_regs *regs,
-		       unsigned long *sp, unsigned long bp);
 extern void show_stack_regs(struct pt_regs *regs);
 extern void __show_regs(struct pt_regs *regs, int all);
 extern unsigned long oops_begin(void);
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 92e8f0a..5f49c04 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -182,12 +182,6 @@ show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
 	dump_trace(task, regs, stack, bp, &print_trace_ops, log_lvl);
 }
 
-void show_trace(struct task_struct *task, struct pt_regs *regs,
-		unsigned long *stack, unsigned long bp)
-{
-	show_trace_log_lvl(task, regs, stack, bp, "");
-}
-
 void show_stack(struct task_struct *task, unsigned long *sp)
 {
 	unsigned long bp = 0;
-- 
2.7.4

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

#1448205 — Re: [PATCH 01/19] x86/dumpstack: remove show_trace()

On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> There are a bewildering array of options for dumping the stack.
> Simplify things a little by removing show_trace(), which is unused.

Reviewed-by: Andy Lutomirski <luto@kernel.org>

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

#1448185 — [PATCH 12/19] perf/x86: convert perf_callchain_kernel() to the new unwinder

Convert perf_callchain_kernel() to the new unwinder.  dump_trace() has
been deprecated.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/events/core.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index f388f57..e91b9c3 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -37,6 +37,7 @@
 #include <asm/timer.h>
 #include <asm/desc.h>
 #include <asm/ldt.h>
+#include <asm/unwind.h>
 
 #include "perf_event.h"
 
@@ -2244,31 +2245,12 @@ void arch_perf_update_userpage(struct perf_event *event,
 	cyc2ns_read_end(data);
 }
 
-/*
- * callchain support
- */
-
-static int backtrace_stack(void *data, const char *name)
-{
-	return 0;
-}
-
-static int backtrace_address(void *data, unsigned long addr, int reliable)
-{
-	struct perf_callchain_entry_ctx *entry = data;
-
-	return perf_callchain_store(entry, addr);
-}
-
-static const struct stacktrace_ops backtrace_ops = {
-	.stack			= backtrace_stack,
-	.address		= backtrace_address,
-	.walk_stack		= print_context_stack_bp,
-};
-
 void
 perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs)
 {
+	struct unwind_state state;
+	unsigned long addr;
+
 	if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
 		/* TODO: We don't support guest os callchain now */
 		return;
@@ -2276,7 +2258,11 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
 
 	perf_callchain_store(entry, regs->ip);
 
-	dump_trace(NULL, regs, NULL, 0, &backtrace_ops, entry);
+	for (unwind_start(&state, NULL, regs, NULL); !unwind_done(&state);
+	     unwind_next_frame(&state)) {
+		addr = unwind_get_return_address(&state);
+		perf_callchain_store(entry, addr);
+	}
 }
 
 static inline int
-- 
2.7.4

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

#1448186 — [PATCH 02/19] x86/dumpstack: add get_stack_pointer() and get_frame_pointer()

The various functions involved in dumping the stack all do similar
things with regard to getting the stack pointer and the frame pointer
based on the regs and task arguments.  Create helper functions to
do that instead.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/stacktrace.h | 39 ++++++++++++++++++++++-----------------
 arch/x86/kernel/dumpstack.c       |  5 ++---
 arch/x86/kernel/dumpstack_32.c    | 25 ++++---------------------
 arch/x86/kernel/dumpstack_64.c    | 30 ++++--------------------------
 4 files changed, 32 insertions(+), 67 deletions(-)

diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
index 0944218..6f65995 100644
--- a/arch/x86/include/asm/stacktrace.h
+++ b/arch/x86/include/asm/stacktrace.h
@@ -49,37 +49,42 @@ void dump_trace(struct task_struct *tsk, struct pt_regs *regs,
 
 #ifdef CONFIG_X86_32
 #define STACKSLOTS_PER_LINE 8
-#define get_bp(bp) asm("movl %%ebp, %0" : "=r" (bp) :)
 #else
 #define STACKSLOTS_PER_LINE 4
-#define get_bp(bp) asm("movq %%rbp, %0" : "=r" (bp) :)
 #endif
 
 #ifdef CONFIG_FRAME_POINTER
-static inline unsigned long
-stack_frame(struct task_struct *task, struct pt_regs *regs)
+static inline unsigned long *
+get_frame_pointer(struct task_struct *task, struct pt_regs *regs)
 {
-	unsigned long bp;
-
 	if (regs)
-		return regs->bp;
+		return (unsigned long *)regs->bp;
 
-	if (task == current) {
-		/* Grab bp right from our regs */
-		get_bp(bp);
-		return bp;
-	}
+	if (!task || task == current)
+		return __builtin_frame_address(0);
 
 	/* bp is the last reg pushed by switch_to */
-	return *(unsigned long *)task->thread.sp;
+	return (unsigned long *)*(unsigned long *)task->thread.sp;
 }
 #else
-static inline unsigned long
-stack_frame(struct task_struct *task, struct pt_regs *regs)
+static inline unsigned long *
+get_frame_pointer(struct task_struct *task, struct pt_regs *regs)
 {
 	return 0;
 }
-#endif
+#endif /* CONFIG_FRAME_POINTER */
+
+static inline unsigned long *
+get_stack_pointer(struct task_struct *task, struct pt_regs *regs)
+{
+	if (regs)
+		return (unsigned long *)kernel_stack_pointer(regs);
+
+	if (!task || task == current)
+		return __builtin_frame_address(0);
+
+	return (unsigned long *)task->thread.sp;
+}
 
 extern void
 show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
@@ -106,7 +111,7 @@ static inline unsigned long caller_frame_pointer(void)
 {
 	struct stack_frame *frame;
 
-	get_bp(frame);
+	frame = __builtin_frame_address(0);
 
 #ifdef CONFIG_FRAME_POINTER
 	frame = frame->next_frame;
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 5f49c04..145f18b 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -185,15 +185,14 @@ show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,
 void show_stack(struct task_struct *task, unsigned long *sp)
 {
 	unsigned long bp = 0;
-	unsigned long stack;
 
 	/*
 	 * Stack frames below this one aren't interesting.  Don't show them
 	 * if we're printing for %current.
 	 */
 	if (!sp && (!task || task == current)) {
-		sp = &stack;
-		bp = stack_frame(current, NULL);
+		sp = get_stack_pointer(current, NULL);
+		bp = (unsigned long)get_frame_pointer(current, NULL);
 	}
 
 	show_stack_log_lvl(task, NULL, sp, bp, "");
diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index 0967571..358fe1c 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -46,19 +46,9 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 	int graph = 0;
 	u32 *prev_esp;
 
-	if (!task)
-		task = current;
-
-	if (!stack) {
-		unsigned long dummy;
-
-		stack = &dummy;
-		if (task != current)
-			stack = (unsigned long *)task->thread.sp;
-	}
-
-	if (!bp)
-		bp = stack_frame(task, regs);
+	task = task ? : current;
+	stack = stack ? : get_stack_pointer(task, regs);
+	bp = bp ? : (unsigned long)get_frame_pointer(task, regs);
 
 	for (;;) {
 		void *end_stack;
@@ -95,14 +85,7 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 	unsigned long *stack;
 	int i;
 
-	if (sp == NULL) {
-		if (regs)
-			sp = (unsigned long *)regs->sp;
-		else if (task)
-			sp = (unsigned long *)task->thread.sp;
-		else
-			sp = (unsigned long *)&sp;
-	}
+	sp = sp ? : get_stack_pointer(task, regs);
 
 	stack = sp;
 	for (i = 0; i < kstack_depth_to_print; i++) {
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 9ee4520..bc08e8b 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -154,25 +154,14 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 {
 	const unsigned cpu = get_cpu();
 	unsigned long *irq_stack = (unsigned long *)per_cpu(irq_stack_ptr, cpu);
-	unsigned long dummy;
 	unsigned used = 0;
 	int graph = 0;
 	int done = 0;
 
-	if (!task)
-		task = current;
+	task = task ? : current;
+	stack = stack ? : get_stack_pointer(task, regs);
+	bp = bp ? : (unsigned long)get_frame_pointer(task, regs);
 
-	if (!stack) {
-		if (regs)
-			stack = (unsigned long *)regs->sp;
-		else if (task != current)
-			stack = (unsigned long *)task->thread.sp;
-		else
-			stack = &dummy;
-	}
-
-	if (!bp)
-		bp = stack_frame(task, regs);
 	/*
 	 * Print function call entries in all stacks, starting at the
 	 * current stack address. If the stacks consist of nested
@@ -259,18 +248,7 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 	irq_stack_end	= (unsigned long *)(per_cpu(irq_stack_ptr, cpu));
 	irq_stack	= (unsigned long *)(per_cpu(irq_stack_ptr, cpu) - IRQ_STACK_SIZE);
 
-	/*
-	 * Debugging aid: "show_stack(NULL, NULL);" prints the
-	 * back trace for this cpu:
-	 */
-	if (sp == NULL) {
-		if (regs)
-			sp = (unsigned long *)regs->sp;
-		else if (task)
-			sp = (unsigned long *)task->thread.sp;
-		else
-			sp = (unsigned long *)&sp;
-	}
+	sp = sp ? : get_stack_pointer(task, regs);
 
 	stack = sp;
 	for (i = 0; i < kstack_depth_to_print; i++) {
-- 
2.7.4

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

#1448201 — Re: [PATCH 02/19] x86/dumpstack: add get_stack_pointer() and get_frame_pointer()

On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> The various functions involved in dumping the stack all do similar
> things with regard to getting the stack pointer and the frame pointer
> based on the regs and task arguments.  Create helper functions to
> do that instead.
>

Reviewed-by: Andy Lutomirski <luto@kernel.org>

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

#1448188 — [PATCH 11/19] x86/dumptrace: add new unwind interface and implementations

The x86 stack dump code is a bit of a mess.  dump_trace() uses
callbacks, and each user of it seems to have slightly different
requirements, so there are several slightly different callbacks floating
around.

Also there are some upcoming features which will require more changes to
the stack dump code: reliable stack detection for live patching,
hardened user copy, and the DWARF unwinder.  Each of those features
would at least need more callbacks and/or callback interfaces, resulting
in a much bigger mess than what we have today.

Before doing all that, we should try to clean things up and replace
dump_trace() with something cleaner and more flexible.

The new unwinder is a simple state machine which was heavily inspired by
a suggestion from Andy Lutomirski:

  https://lkml.kernel.org/r/CALCETrUbNTqaM2LRyXGRx=kVLRPeY5A3Pc6k4TtQxF320rUT=w@mail.gmail.com

It's also very similar to the libunwind API:

  http://www.nongnu.org/libunwind/man/libunwind(3).html

Some if its advantages:

- Simplicity: no more callback sprawl and less code duplication.

- Flexibility: it allows the caller to stop and inspect the stack state
  at each step in the unwinding process.

- Modularity: the unwinder code, console stack dump code, and stack
  metadata analysis code are all better separated so that changing one
  of them shouldn't have much of an impact on any of the others.

Two implementations are added which conform to the new unwind interface:

- The frame pointer unwinder which is used for CONFIG_FRAME_POINTER=y.

- The "guess" unwinder which is used for CONFIG_FRAME_POINTER=n.  This
  isn't an "unwinder" per se.  All it does is scan the stack for kernel
  text addresses.  But with no frame pointers, guesses are better than
  nothing in most cases.

Suggested-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/unwind.h  | 80 +++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/Makefile       |  6 +++
 arch/x86/kernel/unwind_frame.c | 89 ++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/unwind_guess.c | 40 +++++++++++++++++++
 4 files changed, 215 insertions(+)
 create mode 100644 arch/x86/include/asm/unwind.h
 create mode 100644 arch/x86/kernel/unwind_frame.c
 create mode 100644 arch/x86/kernel/unwind_guess.c

diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h
new file mode 100644
index 0000000..61c6e95
--- /dev/null
+++ b/arch/x86/include/asm/unwind.h
@@ -0,0 +1,80 @@
+#ifndef _ASM_X86_UNWIND_H
+#define _ASM_X86_UNWIND_H
+
+#include <linux/sched.h>
+#include <linux/ftrace.h>
+#include <asm/ptrace.h>
+#include <asm/stacktrace.h>
+
+struct unwind_state {
+	struct stack_info stack_info;
+	unsigned long stack_mask;
+	struct task_struct *task;
+	unsigned long *sp;
+	int graph_idx;
+#ifdef CONFIG_FRAME_POINTER
+	unsigned long *bp;
+#endif
+};
+
+void __unwind_start(struct unwind_state *state, struct task_struct *task,
+		    struct pt_regs *regs, unsigned long *sp);
+
+bool unwind_next_frame(struct unwind_state *state);
+
+
+#ifdef CONFIG_FRAME_POINTER
+
+static inline unsigned long *unwind_get_return_address_ptr(struct unwind_state *state)
+{
+	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
+		return NULL;
+
+	return state->bp + 1;
+}
+
+unsigned long unwind_get_return_address(struct unwind_state *state);
+
+#else /* !CONFIG_FRAME_POINTER */
+
+static inline unsigned long *unwind_get_return_address_ptr(struct unwind_state *state)
+{
+	return NULL;
+}
+
+static inline unsigned long unwind_get_return_address(struct unwind_state *state)
+{
+	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
+		return 0;
+
+	return *state->sp;
+}
+
+#endif /* CONFIG_FRAME_POINTER */
+
+static inline unsigned long *unwind_get_stack_ptr(struct unwind_state *state)
+{
+	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
+		return NULL;
+
+	return state->sp;
+}
+
+static inline bool unwind_done(struct unwind_state *state)
+{
+	return (state->stack_info.type == STACK_TYPE_UNKNOWN);
+}
+
+static inline
+void unwind_start(struct unwind_state *state, struct task_struct *task,
+		  struct pt_regs *regs, unsigned long *sp)
+{
+	if (!task)
+		task = current;
+
+	sp = sp ? : get_stack_pointer(task, regs);
+
+	__unwind_start(state, task, regs, sp);
+}
+
+#endif /* _ASM_X86_UNWIND_H */
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 0503f5b..45257cf 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -125,6 +125,12 @@ obj-$(CONFIG_EFI)			+= sysfb_efi.o
 obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o
 obj-$(CONFIG_TRACING)			+= tracepoint.o
 
+ifdef CONFIG_FRAME_POINTER
+obj-y					+= unwind_frame.o
+else
+obj-y					+= unwind_guess.o
+endif
+
 ###
 # 64 bit specific files
 ifeq ($(CONFIG_X86_64),y)
diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
new file mode 100644
index 0000000..1234480
--- /dev/null
+++ b/arch/x86/kernel/unwind_frame.c
@@ -0,0 +1,89 @@
+#include <linux/sched.h>
+#include <asm/ptrace.h>
+#include <asm/bitops.h>
+#include <asm/stacktrace.h>
+#include <asm/unwind.h>
+
+unsigned long unwind_get_return_address(struct unwind_state *state)
+{
+	unsigned long addr, graph_addr;
+
+	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
+		return 0;
+
+	addr = *unwind_get_return_address_ptr(state);
+	graph_addr = ftrace_graph_ret_addr(state->task, &state->graph_idx,
+					   addr);
+	return graph_addr ? : addr;
+}
+EXPORT_SYMBOL_GPL(unwind_get_return_address);
+
+static unsigned long *update_stack_state(struct unwind_state *state, void *addr,
+					 size_t len)
+{
+	struct stack_info *info = &state->stack_info;
+	unsigned long *sp;
+
+	if (on_stack(info, addr, len))
+		return addr;
+
+	sp = info->next;
+	if (!sp)
+		goto unknown;
+
+	if (get_stack_info(sp, state->task, info, &state->stack_mask))
+		goto unknown;
+
+	if (!on_stack(info, addr, len))
+		goto unknown;
+
+	return sp;
+
+unknown:
+	info->type = STACK_TYPE_UNKNOWN;
+	return NULL;
+}
+
+static bool unwind_next_frame_bp(struct unwind_state *state, unsigned long *bp)
+{
+	unsigned long *sp;
+
+	sp = update_stack_state(state, bp, sizeof(*bp) * 2);
+	if (state->stack_info.type == STACK_TYPE_UNKNOWN)
+		return false;
+
+	state->bp = bp;
+	state->sp = sp;
+
+	return true;
+}
+
+bool unwind_next_frame(struct unwind_state *state)
+{
+	unsigned long *bp;
+
+	if (unwind_done(state))
+		return false;
+
+	bp = (unsigned long *)*state->bp;
+
+	return unwind_next_frame_bp(state, bp);
+}
+EXPORT_SYMBOL_GPL(unwind_next_frame);
+
+void __unwind_start(struct unwind_state *state, struct task_struct *task,
+		    struct pt_regs *regs, unsigned long *sp)
+{
+	memset(state, 0, sizeof(*state));
+
+	state->task = task;
+	state->sp = sp;
+	state->bp = get_frame_pointer(task, regs);
+
+	get_stack_info(sp, state->task, &state->stack_info, &state->stack_mask);
+
+	/* unwind to the first frame after the user-specified stack pointer */
+	while (state->bp < sp && !unwind_done(state))
+		unwind_next_frame(state);
+}
+EXPORT_SYMBOL_GPL(__unwind_start);
diff --git a/arch/x86/kernel/unwind_guess.c b/arch/x86/kernel/unwind_guess.c
new file mode 100644
index 0000000..223d020
--- /dev/null
+++ b/arch/x86/kernel/unwind_guess.c
@@ -0,0 +1,40 @@
+#include <linux/sched.h>
+#include <linux/ftrace.h>
+#include <asm/ptrace.h>
+#include <asm/bitops.h>
+#include <asm/stacktrace.h>
+#include <asm/unwind.h>
+
+bool unwind_next_frame(struct unwind_state *state)
+{
+	struct stack_info *info = &state->stack_info;
+
+	if (info->type == STACK_TYPE_UNKNOWN)
+		return false;
+
+	do {
+		for (state->sp++; state->sp < info->end; state->sp++)
+			if (__kernel_text_address(*state->sp))
+				return true;
+
+		state->sp = info->next;
+
+	} while (!get_stack_info(state->sp, state->task, info,
+				 &state->stack_mask));
+
+	return false;
+}
+
+void __unwind_start(struct unwind_state *state, struct task_struct *task,
+		    struct pt_regs *regs, unsigned long *sp)
+{
+	memset(state, 0, sizeof(*state));
+
+	state->task = task;
+	state->sp   = sp;
+
+	get_stack_info(sp, state->task, &state->stack_info, &state->stack_mask);
+
+	if (!__kernel_text_address(*sp))
+		unwind_next_frame(state);
+}
-- 
2.7.4

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

#1448189 — [PATCH 09/19] x86/dumpstack: simplify in_exception_stack()

in_exception_stack() does some bad, bad things just so the unwinder can
print different values for different areas of the debug exception stack.

There's no need to clarify where exactly on the stack it is.  Just print
"#DB" and be done with it.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/dumpstack_64.c | 106 +++++++++++++----------------------------
 1 file changed, 32 insertions(+), 74 deletions(-)

diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 634ed22..0641d75 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -16,83 +16,41 @@
 
 #include <asm/stacktrace.h>
 
+static char *exception_stack_names[N_EXCEPTION_STACKS] = {
+		[ DOUBLEFAULT_STACK-1	]	= "#DF",
+		[ NMI_STACK-1		]	= "NMI",
+		[ DEBUG_STACK-1		]	= "#DB",
+		[ MCE_STACK-1		]	= "#MC",
+};
 
-#define N_EXCEPTION_STACKS_END \
-		(N_EXCEPTION_STACKS + DEBUG_STKSZ/EXCEPTION_STKSZ - 2)
-
-static char x86_stack_ids[][8] = {
-		[ DEBUG_STACK-1			]	= "#DB",
-		[ NMI_STACK-1			]	= "NMI",
-		[ DOUBLEFAULT_STACK-1		]	= "#DF",
-		[ MCE_STACK-1			]	= "#MC",
-#if DEBUG_STKSZ > EXCEPTION_STKSZ
-		[ N_EXCEPTION_STACKS ...
-		  N_EXCEPTION_STACKS_END	]	= "#DB[?]"
-#endif
+static unsigned long exception_stack_sizes[N_EXCEPTION_STACKS] = {
+	[0 ... N_EXCEPTION_STACKS - 1]		= EXCEPTION_STKSZ,
+	[DEBUG_STACK - 1]			= DEBUG_STKSZ
 };
 
-static unsigned long *in_exception_stack(unsigned long stack, unsigned *usedp,
-					 char **idp)
+static unsigned long *in_exception_stack(unsigned long *s, char **name,
+					 unsigned long *visit_mask)
 {
+	unsigned long stack = (unsigned long)s;
+	unsigned long begin, end;
 	unsigned k;
 
-	/*
-	 * Iterate over all exception stacks, and figure out whether
-	 * 'stack' is in one of them:
-	 */
+	BUILD_BUG_ON(N_EXCEPTION_STACKS != 4);
+
 	for (k = 0; k < N_EXCEPTION_STACKS; k++) {
-		unsigned long end = this_cpu_ptr(&orig_ist)->ist[k];
-		/*
-		 * Is 'stack' above this exception frame's end?
-		 * If yes then skip to the next frame.
-		 */
-		if (stack >= end)
+		end   = this_cpu_ptr(&orig_ist)->ist[k];
+		begin = end - exception_stack_sizes[k];
+
+		if (stack < begin || stack >= end)
 			continue;
-		/*
-		 * Is 'stack' above this exception frame's start address?
-		 * If yes then we found the right frame.
-		 */
-		if (stack >= end - EXCEPTION_STKSZ) {
-			/*
-			 * Make sure we only iterate through an exception
-			 * stack once. If it comes up for the second time
-			 * then there's something wrong going on - just
-			 * break out and return NULL:
-			 */
-			if (*usedp & (1U << k))
-				break;
-			*usedp |= 1U << k;
-			*idp = x86_stack_ids[k];
-			return (unsigned long *)end;
-		}
-		/*
-		 * If this is a debug stack, and if it has a larger size than
-		 * the usual exception stacks, then 'stack' might still
-		 * be within the lower portion of the debug stack:
-		 */
-#if DEBUG_STKSZ > EXCEPTION_STKSZ
-		if (k == DEBUG_STACK - 1 && stack >= end - DEBUG_STKSZ) {
-			unsigned j = N_EXCEPTION_STACKS - 1;
 
-			/*
-			 * Black magic. A large debug stack is composed of
-			 * multiple exception stack entries, which we
-			 * iterate through now. Dont look:
-			 */
-			do {
-				++j;
-				end -= EXCEPTION_STKSZ;
-				x86_stack_ids[j][4] = '1' +
-						(j - N_EXCEPTION_STACKS);
-			} while (stack < end - EXCEPTION_STKSZ);
-			if (*usedp & (1U << j))
-				break;
-			*usedp |= 1U << j;
-			*idp = x86_stack_ids[j];
-			return (unsigned long *)end;
-		}
-#endif
+		if (test_and_set_bit(k, visit_mask))
+			return false;
+
+		*name = exception_stack_names[k];
+		return (unsigned long *)end;
 	}
+
 	return NULL;
 }
 
@@ -113,7 +71,7 @@ enum stack_type {
 static enum stack_type
 analyze_stack(struct task_struct *task, unsigned long *stack,
 	      unsigned long **stack_end, unsigned long *irq_stack,
-	      unsigned *used, char **id)
+	      unsigned long *visit_mask, char **name)
 {
 	unsigned long addr;
 
@@ -121,7 +79,7 @@ analyze_stack(struct task_struct *task, unsigned long *stack,
 	if ((unsigned long)task_stack_page(task) == addr)
 		return STACK_IS_NORMAL;
 
-	*stack_end = in_exception_stack((unsigned long)stack, used, id);
+	*stack_end = in_exception_stack(stack, name, visit_mask);
 	if (*stack_end)
 		return STACK_IS_EXCEPTION;
 
@@ -149,7 +107,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 		const struct stacktrace_ops *ops, void *data)
 {
 	unsigned long *irq_stack = (unsigned long *)this_cpu_read(irq_stack_ptr);
-	unsigned used = 0;
+	unsigned long visit_mask = 0;
 	int graph = 0;
 	int done = 0;
 
@@ -165,10 +123,10 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 	while (!done) {
 		unsigned long *stack_end;
 		enum stack_type stype;
-		char *id;
+		char *name;
 
-		stype = analyze_stack(task, stack, &stack_end, irq_stack, &used,
-				      &id);
+		stype = analyze_stack(task, stack, &stack_end, irq_stack,
+				      &visit_mask, &name);
 
 		/* Default finish unless specified to continue */
 		done = 1;
@@ -181,7 +139,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 
 		case STACK_IS_EXCEPTION:
 
-			if (ops->stack(data, id) < 0)
+			if (ops->stack(data, name) < 0)
 				break;
 
 			bp = ops->walk_stack(task, stack, bp, ops,
-- 
2.7.4

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

#1448215 — Re: [PATCH 09/19] x86/dumpstack: simplify in_exception_stack()

On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> in_exception_stack() does some bad, bad things just so the unwinder can
> print different values for different areas of the debug exception stack.
>
> There's no need to clarify where exactly on the stack it is.  Just print
> "#DB" and be done with it.

This is a huge improvement.

However: could you add a comment clarifying what purpose visit_mask serves?

FWIW, I have patches that remove the extra debug stacks, and they'll
be nicer on top of this.

--Andy

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

#1448190 — [PATCH 07/19] x86/dumpstack: add IRQ_USABLE_STACK_SIZE define

For reasons unknown, the x86_64 irq stack starts at an offset 64 bytes
from the end of the page.  At least make that explicit.

FIXME: Can we just remove the 64 byte gap?  If not, at least document
why.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/page_64_types.h | 19 +++++++++++--------
 arch/x86/kernel/cpu/common.c         |  2 +-
 arch/x86/kernel/dumpstack_64.c       |  8 +++-----
 arch/x86/kernel/setup_percpu.c       |  2 +-
 4 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h
index 9215e05..6256baf 100644
--- a/arch/x86/include/asm/page_64_types.h
+++ b/arch/x86/include/asm/page_64_types.h
@@ -12,17 +12,20 @@
 #endif
 
 #define THREAD_SIZE_ORDER	(2 + KASAN_STACK_ORDER)
-#define THREAD_SIZE  (PAGE_SIZE << THREAD_SIZE_ORDER)
-#define CURRENT_MASK (~(THREAD_SIZE - 1))
+#define THREAD_SIZE		(PAGE_SIZE << THREAD_SIZE_ORDER)
+#define CURRENT_MASK		(~(THREAD_SIZE - 1))
 
-#define EXCEPTION_STACK_ORDER (0 + KASAN_STACK_ORDER)
-#define EXCEPTION_STKSZ (PAGE_SIZE << EXCEPTION_STACK_ORDER)
+#define EXCEPTION_STACK_ORDER	(0 + KASAN_STACK_ORDER)
+#define EXCEPTION_STKSZ		(PAGE_SIZE << EXCEPTION_STACK_ORDER)
 
-#define DEBUG_STACK_ORDER (EXCEPTION_STACK_ORDER + 1)
-#define DEBUG_STKSZ (PAGE_SIZE << DEBUG_STACK_ORDER)
+#define DEBUG_STACK_ORDER	(EXCEPTION_STACK_ORDER + 1)
+#define DEBUG_STKSZ		(PAGE_SIZE << DEBUG_STACK_ORDER)
 
-#define IRQ_STACK_ORDER (2 + KASAN_STACK_ORDER)
-#define IRQ_STACK_SIZE (PAGE_SIZE << IRQ_STACK_ORDER)
+#define IRQ_STACK_ORDER		(2 + KASAN_STACK_ORDER)
+#define IRQ_STACK_SIZE		(PAGE_SIZE << IRQ_STACK_ORDER)
+
+/* FIXME: why? */
+#define IRQ_USABLE_STACK_SIZE	(IRQ_STACK_SIZE - 64)
 
 #define DOUBLEFAULT_STACK 1
 #define NMI_STACK 2
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 809eda0..8f3f7a4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1281,7 +1281,7 @@ DEFINE_PER_CPU(struct task_struct *, current_task) ____cacheline_aligned =
 EXPORT_PER_CPU_SYMBOL(current_task);
 
 DEFINE_PER_CPU(char *, irq_stack_ptr) =
-	init_per_cpu_var(irq_stack_union.irq_stack) + IRQ_STACK_SIZE - 64;
+	init_per_cpu_var(irq_stack_union.irq_stack) + IRQ_USABLE_STACK_SIZE;
 
 DEFINE_PER_CPU(unsigned int, irq_count) __visible = -1;
 
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 55cc88f..6a2d14e 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -103,9 +103,6 @@ in_irq_stack(unsigned long *stack, unsigned long *irq_stack,
 	return (stack >= irq_stack && stack < irq_stack_end);
 }
 
-static const unsigned long irq_stack_size =
-	(IRQ_STACK_SIZE - 64) / sizeof(unsigned long);
-
 enum stack_type {
 	STACK_IS_UNKNOWN,
 	STACK_IS_NORMAL,
@@ -133,7 +130,7 @@ analyze_stack(int cpu, struct task_struct *task, unsigned long *stack,
 		return STACK_IS_NORMAL;
 
 	*stack_end = irq_stack;
-	irq_stack = irq_stack - irq_stack_size;
+	irq_stack -= (IRQ_USABLE_STACK_SIZE / sizeof(long));
 
 	if (in_irq_stack(stack, irq_stack, *stack_end))
 		return STACK_IS_IRQ;
@@ -246,7 +243,8 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 	cpu = smp_processor_id();
 
 	irq_stack_end	= (unsigned long *)(per_cpu(irq_stack_ptr, cpu));
-	irq_stack	= (unsigned long *)(per_cpu(irq_stack_ptr, cpu) - IRQ_STACK_SIZE);
+	irq_stack	= (unsigned long *)(per_cpu(irq_stack_ptr, cpu) -
+			  IRQ_USABLE_STACK_SIZE);
 
 	sp = sp ? : get_stack_pointer(task, regs);
 
diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index e4fcb87..043454f 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -244,7 +244,7 @@ void __init setup_per_cpu_areas(void)
 #ifdef CONFIG_X86_64
 		per_cpu(irq_stack_ptr, cpu) =
 			per_cpu(irq_stack_union.irq_stack, cpu) +
-			IRQ_STACK_SIZE - 64;
+			IRQ_USABLE_STACK_SIZE;
 #endif
 #ifdef CONFIG_NUMA
 		per_cpu(x86_cpu_to_node_map, cpu) =
-- 
2.7.4

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

#1448213 — Re: [PATCH 07/19] x86/dumpstack: add IRQ_USABLE_STACK_SIZE define

On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> For reasons unknown, the x86_64 irq stack starts at an offset 64 bytes
> from the end of the page.  At least make that explicit.

This is a change in behavior -- see below.  Please mention this in the
changelog.

>
> FIXME: Can we just remove the 64 byte gap?  If not, at least document
> why.

I have no clue.

>
>         irq_stack_end   = (unsigned long *)(per_cpu(irq_stack_ptr, cpu));
> -       irq_stack       = (unsigned long *)(per_cpu(irq_stack_ptr, cpu) - IRQ_STACK_SIZE);
> +       irq_stack       = (unsigned long *)(per_cpu(irq_stack_ptr, cpu) -
> +                         IRQ_USABLE_STACK_SIZE);

This is different.

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

#1448296 — Re: [PATCH 07/19] x86/dumpstack: add IRQ_USABLE_STACK_SIZE define

On Thu, Jul 21, 2016 at 03:01:10PM -0700, Andy Lutomirski wrote:
> On Thu, Jul 21, 2016 at 2:21 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > For reasons unknown, the x86_64 irq stack starts at an offset 64 bytes
> > from the end of the page.  At least make that explicit.
> 
> This is a change in behavior -- see below.  Please mention this in the
> changelog.

Ah, right.

> 
> >
> > FIXME: Can we just remove the 64 byte gap?  If not, at least document
> > why.
> 
> I have no clue.
> 
> >
> >         irq_stack_end   = (unsigned long *)(per_cpu(irq_stack_ptr, cpu));
> > -       irq_stack       = (unsigned long *)(per_cpu(irq_stack_ptr, cpu) - IRQ_STACK_SIZE);
> > +       irq_stack       = (unsigned long *)(per_cpu(irq_stack_ptr, cpu) -
> > +                         IRQ_USABLE_STACK_SIZE);
> 
> This is different.

If nobody knows the reason for it, I may just remove it.  It doesn't
seem to blow anything up on my system.  I tried digging through the git
history but it's been there since the beginning of git time.

-- 
Josh

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

#1448458 — Re: [PATCH 07/19] x86/dumpstack: add IRQ_USABLE_STACK_SIZE define

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> > >         irq_stack_end   = (unsigned long *)(per_cpu(irq_stack_ptr, cpu));
> > > -       irq_stack       = (unsigned long *)(per_cpu(irq_stack_ptr, cpu) - IRQ_STACK_SIZE);
> > > +       irq_stack       = (unsigned long *)(per_cpu(irq_stack_ptr, cpu) -
> > > +                         IRQ_USABLE_STACK_SIZE);
> > 
> > This is different.
> 
> If nobody knows the reason for it, I may just remove it.  It doesn't
> seem to blow anything up on my system.  I tried digging through the git
> history but it's been there since the beginning of git time.

Please do any behavioral changes in separate patches - ordered after all the 'does 
not change behavior' low-risk patches.

I.e. try to order the patches by risk: (near-)zero-risk ones first, followed by 
lower risk ones, closed by higher risk ones. This makes both review, application 
of the patches and any bisection/fixing work later on easier.

If you ever see a good chance to split a patch that changes behavior into a 
zero-risk and a low-risk component, do so - we'd rather err on the side of being 
too finegrained in a series than having to scratch heads when bisecting to overly 
large patches.

Thanks,

	Ingo

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

#1448851

On Fri, Jul 22, 2016 at 6:21 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Some if its advantages:
>
> - simplicity: no more callback sprawl and less code duplication.
>
> - flexibility: allows the caller to stop and inspect the stack state at
>   each step in the unwinding process.
>
> - modularity: the unwinder code, console stack dump code, and stack
>   metadata analysis code are all better separated so that changing one
>   of them shouldn't have much of an impact on any of the others.

I've been without internet for the last week, so I have a ton pending,
and not good enough internet even now to take a good look.

However, I want to make one thing really really clear: the absolute
NUMBER ONE requirement for the stack tracing code is none of the
above.

The #1 requirement is that it works, and not have a chance in hell of
ever breaking. We had that happen once before when people wanted to
make it fancy and add Dwarf info, and it was such a f*cking disaster
that I am not sure I ever want to do that again. Seriously.

It does not matter if the stack tracing gives the wrong answers.

It does not matter if the stack tracing is complicated and odd old code.

It does not matter one whit if some new user is inconvenienced, and in
fact it is possible that new users should write their *own* stack
tracer code.

The ONLY thing that matters (to a very high degree) is that the code
is stable, and if an Oops happens, the stack tracer never *ever*
cause even more problems than we already have.

If the stack tracer *ever* takes a recursive fault and kills the
machine, the stack tracer is worse than bad - we'd be better off
*without* a stack tracer at all.

And yes, we had exactly that situation, where bugs in the stack tracer
meant that other bugs ended up being much harder to debug, because
instead of a nice logged oops message that would have been trivial to
figure out, we very occasionally ended up with a dead machine instead.

So without having yet looked at the code, I want people to understand
that to a very real degree, the stack tracer that the *oopsing* code
(ie what all the usual kernel fault handlers use) is very very special
code and needs to be handled very carefully, and needs to be extra
robust, even in the presence of stack corruption, and even in the
presence of the dwarf info being totally corrupted. Because we've very
much had both things happen.

It is very possible that we should have two different stack tracers -
the stupid "for oopses only" code that doesn't necessarily give the
perfect trace, but is very anal and happily gives old stale addresses
(which can be very useful for seeing what happened just before the
"real" stack trace), and then a separate stack trace engine that is
clever and gets things right, and if that one faults it can depend on
the normal kernel fault handling picking up the pieces.

Yes, the current stack tracer is  crufty. No, it's not perfect. But it
is very well tested, and has held up. That should not be dismissed.

                 Linus

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

#1448852

On Fri, Jul 22, 2016 at 5:22 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So without having yet looked at the code, I want people to understand
> that to a very real degree, the stack tracer that the *oopsing* code
> (ie what all the usual kernel fault handlers use) is very very special
> code and needs to be handled very carefully, and needs to be extra
> robust, even in the presence of stack corruption, and even in the
> presence of the dwarf info being totally corrupted. Because we've very
> much had both things happen.
>
> It is very possible that we should have two different stack tracers -
> the stupid "for oopses only" code that doesn't necessarily give the
> perfect trace, but is very anal and happily gives old stale addresses
> (which can be very useful for seeing what happened just before the
> "real" stack trace), and then a separate stack trace engine that is
> clever and gets things right, and if that one faults it can depend on
> the normal kernel fault handling picking up the pieces.

I think that Josh's code has the potential to be extremely robust
*and* give more correct results when possible.  One thing I intend to
review when v2 shows up is that it's as conservative as it needs to be
to avoid ever dereferencing an out-of-bounds pointer.  And Josh's oops
printer carefully walks and prints out all addresses on the stack
(complete with question marks) even if the unwinder doesn't find them.

>
> Yes, the current stack tracer is  crufty. No, it's not perfect. But it
> is very well tested, and has held up. That should not be dismissed.
>

I think you may be giving the current tracer slightly more credit than
it's due.  In my stack guard page patchset, I fixed two separate
issues, one of which caused recursive faults and one of which caused
it to output nothing at all.  So maybe *now* it's very robust :)  But
it's still an umaintainable mess IMO, and Josh's patchset helps a
*lot*.

--Andy

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

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web