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

Groups > linux.kernel > #1461224 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 00/51] x86/dumpstack: rewrite x86 stack dump code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 49/51] x86/dumpstack: warn on stack recursion Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 36/51] perf/x86: convert perf_callchain_kernel() to use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 26/51] ftrace: add ftrace_graph_ret_addr() stack unwinding helpers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 33/51] x86/dumpstack: add get_stack_info() interface Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 48/51] x86/unwind: warn if stack grows up Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 48/51] x86/unwind: warn if stack grows up Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:30 +0200
        Re: [PATCH v3 48/51] x86/unwind: warn if stack grows up Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 18:30 +0200
    [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Kees Cook <keescook@chromium.org> - 2016-08-12 19:40 +0200
        Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to  usercopy.c Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 21:20 +0200
          Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Kees Cook <keescook@chromium.org> - 2016-08-12 22:10 +0200
            Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to  usercopy.c Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 22:40 +0200
              Re: [PATCH v3 50/51] x86/mm: move arch_within_stack_frames() to usercopy.c Kees Cook <keescook@chromium.org> - 2016-08-12 22:50 +0200
    [PATCH v3 44/51] x86/dumpstack: print any pt_regs found on the stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 44/51] x86/dumpstack: print any pt_regs found on the stack Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:50 +0200
    [PATCH v3 12/51] x86: move _stext marker to before head code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 32/51] x86/dumpstack: simplify in_exception_stack() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 32/51] x86/dumpstack: simplify in_exception_stack() Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:50 +0200
        Re: [PATCH v3 32/51] x86/dumpstack: simplify in_exception_stack() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:40 +0200
    [PATCH v3 14/51] x86/asm/head:  put real return address on idle task stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 14/51] x86/asm/head: put real return address on idle  task stack Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:40 +0200
      Re: [PATCH v3 14/51] x86/asm/head: put real return address on idle  task stack Nilay Vaish <nilayvaish@gmail.com> - 2016-08-17 22:40 +0200
        Re: [PATCH v3 14/51] x86/asm/head: put real return address on idle  task stack Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-17 23:20 +0200
    [PATCH v3 16/51] x86/32: put real return address on stack in entry code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 16/51] x86/32: put real return address on stack in  entry code Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:30 +0200
        Re: [PATCH v3 16/51] x86/32: put real return address on stack in  entry code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:10 +0200
          Re: [PATCH v3 16/51] x86/32: put real return address on stack in  entry code "H. Peter Anvin" <hpa@zytor.com> - 2016-08-15 20:10 +0200
            Re: [PATCH v3 16/51] x86/32: put real return address on stack in  entry code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 20:30 +0200
              Re: [PATCH v3 16/51] x86/32: put real return address on stack in  entry code "H. Peter Anvin" <hpa@zytor.com> - 2016-08-15 21:30 +0200
                Re: [PATCH v3 16/51] x86/32: put real return address on stack in  entry code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 22:10 +0200
    [PATCH v3 18/51] x86/entry/head/32: use local labels Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 30/51] x86/dumpstack/ftrace: don't print unreliable addresses in print_context_stack_bp() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 42/51] x86/unwind: create stack frames for saved syscall registers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 42/51] x86/unwind: create stack frames for saved  syscall registers Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:40 +0200
        Re: [PATCH v3 42/51] x86/unwind: create stack frames for saved  syscall registers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 19:00 +0200
    [PATCH v3 28/51] ftrace/x86: implement HAVE_FUNCTION_GRAPH_RET_ADDR_PTR Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 31/51] x86/dumpstack: allow preemption in show_stack_log_lvl() and dump_trace() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 31/51] x86/dumpstack: allow preemption in  show_stack_log_lvl() and dump_trace() Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:50 +0200
        Re: [PATCH v3 31/51] x86/dumpstack: allow preemption in  show_stack_log_lvl() and dump_trace() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:40 +0200
    [PATCH v3 41/51] x86/entry/unwind: create stack frames for saved interrupt registers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
      Re: [PATCH v3 41/51] x86/entry/unwind: create stack frames for saved  interrupt registers Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:40 +0200
        Re: [PATCH v3 41/51] x86/entry/unwind: create stack frames for saved  interrupt registers Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 18:40 +0200
    [PATCH v3 37/51] x86/stacktrace: convert save_stack_trace_*() to use the new unwinder Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 05/51] x86/dumpstack: make printk_stack_address() more generally useful Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 43/51] x86/dumpstack: print stack identifier on its own line Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 24/51] ftrace: only allocate the ret_stack 'fp' field when needed Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 29/51] x86/dumpstack/ftrace: mark function graph handler function as unreliable Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:40 +0200
    [PATCH v3 03/51] x86/asm/head: rename 'stack_start' -> 'initial_stack' Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer() previous stack access Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
      Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer()  previous stack access Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:30 +0200
        Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer()  previous stack access Brian Gerst <brgerst@gmail.com> - 2016-08-14 15:00 +0200
          Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer()  previous stack access Andy Lutomirski <luto@amacapital.net> - 2016-08-14 15:50 +0200
        Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32  kernel_stack_pointer() previous stack access Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:10 +0200
          Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32  kernel_stack_pointer() previous stack access Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 19:30 +0200
            Re: [PATCH v3 09/51] x86/dumpstack: fix x86_32 kernel_stack_pointer()  previous stack access Andy Lutomirski <luto@amacapital.net> - 2016-08-15 22:10 +0200
    [PATCH v3 13/51] x86/asm/head: remove useless zeroed word Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 21/51] oprofile/x86: add regs->ip to oprofile trace Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 10/51] x86/dumpstack: add get_stack_pointer() and get_frame_pointer() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 07/51] x86/dumpstack: remove extra brackets around "<EOE>" Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 17/51] x86/smp: fix initial idle stack location on 32-bit Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 08/51] x86/dumpstack: fix irq stack bounds calculation in show_stack_log_lvl() Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 15/51] x86/asm/head: standardize the end of the stack for idle tasks Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
      Re: [PATCH v3 15/51] x86/asm/head: standardize the end of the stack  for idle tasks Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:40 +0200
    [PATCH v3 19/51] x86/entry/32: rename 'error_code' to 'common_exception' Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
      Re: [PATCH v3 19/51] x86/entry/32: rename 'error_code' to 'common_exception' Andy Lutomirski <luto@amacapital.net> - 2016-08-14 10:50 +0200
        Re: [PATCH v3 19/51] x86/entry/32: rename 'error_code' to  'common_exception' Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-15 17:40 +0200
    [PATCH v3 23/51] ftrace: remove CONFIG_HAVE_FUNCTION_GRAPH_FP_TEST from config Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 16:50 +0200
    [PATCH v3 11/51] x86/dumpstack: remove unnecessary stack pointer arguments Josh Poimboeuf <jpoimboe@redhat.com> - 2016-08-12 17:10 +0200

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

#1461236 — [PATCH v3 14/51] x86/asm/head: put real return address on idle task stack

The frame at the end of each idle task stack has a zeroed return
address.  This is inconsistent with real task stacks, which have a real
return address at that spot.  This inconsistency can be confusing for
stack unwinders.

Make it a real address by using the side effect of a call instruction to
push the instruction pointer on the stack.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/head_64.S | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 3621ad2..c90f481 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -298,8 +298,9 @@ ENTRY(start_cpu)
 	 *	REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
 	 *		address given in m16:64.
 	 */
-	movq	initial_code(%rip),%rax
-	pushq	$0		# fake return address to stop unwinder
+	call	1f		# put return address on stack for unwinder
+1:	xorq	%rbp, %rbp	# clear frame pointer
+	movq	initial_code(%rip), %rax
 	pushq	$__KERNEL_CS	# set correct cs
 	pushq	%rax		# target address in negative space
 	lretq
-- 
2.7.4

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

#1461694 — Re: [PATCH v3 14/51] x86/asm/head: put real return address on idle task stack

On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> The frame at the end of each idle task stack has a zeroed return
> address.  This is inconsistent with real task stacks, which have a real
> return address at that spot.  This inconsistency can be confusing for
> stack unwinders.
>
> Make it a real address by using the side effect of a call instruction to
> push the instruction pointer on the stack.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/kernel/head_64.S | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 3621ad2..c90f481 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -298,8 +298,9 @@ ENTRY(start_cpu)
>          *      REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
>          *              address given in m16:64.
>          */
> -       movq    initial_code(%rip),%rax
> -       pushq   $0              # fake return address to stop unwinder
> +       call    1f              # put return address on stack for unwinder
> +1:     xorq    %rbp, %rbp      # clear frame pointer
> +       movq    initial_code(%rip), %rax
>         pushq   $__KERNEL_CS    # set correct cs
>         pushq   %rax            # target address in negative space
>         lretq
> --
> 2.7.4
>

Seems reasonable.

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

--Andy

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

#1464791 — Re: [PATCH v3 14/51] x86/asm/head: put real return address on idle task stack

On 12 August 2016 at 09:28, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> The frame at the end of each idle task stack has a zeroed return
> address.  This is inconsistent with real task stacks, which have a real
> return address at that spot.  This inconsistency can be confusing for
> stack unwinders.
>
> Make it a real address by using the side effect of a call instruction to
> push the instruction pointer on the stack.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/kernel/head_64.S | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 3621ad2..c90f481 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -298,8 +298,9 @@ ENTRY(start_cpu)
>          *      REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
>          *              address given in m16:64.
>          */
> -       movq    initial_code(%rip),%rax
> -       pushq   $0              # fake return address to stop unwinder
> +       call    1f              # put return address on stack for unwinder
> +1:     xorq    %rbp, %rbp      # clear frame pointer
> +       movq    initial_code(%rip), %rax
>         pushq   $__KERNEL_CS    # set correct cs
>         pushq   %rax            # target address in negative space
>         lretq


Josh,  I have a couple of questions.

It seems to me that this patch and the patch 16/51 are both aiming at
the same thing, but are for two different architectures: 32-bit and
64-bit versions of x86.  But you have taken slightly different
approaches in the two patches (for 64-bit, we first jump and then make
a function call, for 32-bit we directly call the function).  Is there
any particular reason for this?  May be I missed out on something.

Second, this is for the whole patch series.  If I wanted to test this
series, how should I go about doing so?

Thanks
Nilay

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

#1464804 — Re: [PATCH v3 14/51] x86/asm/head: put real return address on idle task stack

On Wed, Aug 17, 2016 at 03:30:55PM -0500, Nilay Vaish wrote:
> On 12 August 2016 at 09:28, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > The frame at the end of each idle task stack has a zeroed return
> > address.  This is inconsistent with real task stacks, which have a real
> > return address at that spot.  This inconsistency can be confusing for
> > stack unwinders.
> >
> > Make it a real address by using the side effect of a call instruction to
> > push the instruction pointer on the stack.
> >
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  arch/x86/kernel/head_64.S | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> > index 3621ad2..c90f481 100644
> > --- a/arch/x86/kernel/head_64.S
> > +++ b/arch/x86/kernel/head_64.S
> > @@ -298,8 +298,9 @@ ENTRY(start_cpu)
> >          *      REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
> >          *              address given in m16:64.
> >          */
> > -       movq    initial_code(%rip),%rax
> > -       pushq   $0              # fake return address to stop unwinder
> > +       call    1f              # put return address on stack for unwinder
> > +1:     xorq    %rbp, %rbp      # clear frame pointer
> > +       movq    initial_code(%rip), %rax
> >         pushq   $__KERNEL_CS    # set correct cs
> >         pushq   %rax            # target address in negative space
> >         lretq
> 
> 
> Josh,  I have a couple of questions.
> 
> It seems to me that this patch and the patch 16/51 are both aiming at
> the same thing, but are for two different architectures: 32-bit and
> 64-bit versions of x86.  But you have taken slightly different
> approaches in the two patches (for 64-bit, we first jump and then make
> a function call, for 32-bit we directly call the function).  Is there
> any particular reason for this?  May be I missed out on something.

Yes, the 64-bit code is different: it has to use a far return (lretq) in
order to jump to the 64-bit address stored in 'initial_code'.

So instead of calling the initial code directly, I had to use a more
obtuse approach there ("call 1f") which just places a start_cpu()
address at the right place on the stack before the lretq (which
"returns" to the intial code).

> Second, this is for the whole patch series.  If I wanted to test this
> series, how should I go about doing so?

That's a loaded question. :-) I'd recommend doing lots of stack dumps
and traces in various situations and making sure everything looks sane,
on both 32-bit and 64-bit, and with both CONFIG_FRAME_POINTER=n and
CONFIG_FRAME_POINTER=y.  Some easy ones are:

  cat /proc/*/stack
  echo 1 > /proc/sys/kernel/sysrq; echo l > /proc/sysrq-trigger
  echo 1 > /proc/sys/kernel/sysrq; echo t > /proc/sysrq-trigger
  perf record -g <cmd>; perf report

Also, if you have CONFIG_LOCKDEP enabled, it will silently save a bunch
of stacks in the background, though you won't ever see any evidence of
that unless something goes wrong (you can check if the unwinder spit out
any warnings with 'dmesg |grep WARNING').

For my own testing, I also hacked up my kernel to dump the stack in
various scenarios: interrupts, NMIs, page faults, preemption, perf
callchain record, etc.

BTW, I'll be posting v4 soon, probably Thursday morning US Central time.

-- 
Josh

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

#1461237 — [PATCH v3 16/51] x86/32: put real return address on stack in entry code

This standardizes the stacks of idle tasks to be consistent with other
tasks on 32-bit.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/head_32.S | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index f2298e9..6fc4f1d 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -290,7 +290,7 @@ num_subarch_entries = (. - subarch_entries) / 4
 ENTRY(start_cpu0)
 	movl initial_stack, %ecx
 	movl %ecx, %esp
-	jmp  *(initial_code)
+	call *(initial_code)
 ENDPROC(start_cpu0)
 #endif
 
@@ -471,8 +471,7 @@ is486:
 	xorl %eax,%eax			# Clear LDT
 	lldt %ax
 
-	pushl $0		# fake return address for unwinder
-	jmp *(initial_code)
+	call *(initial_code)
 
 #include "verify_cpu.S"
 
-- 
2.7.4

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

#1461691 — Re: [PATCH v3 16/51] x86/32: put real return address on stack in entry code

On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> This standardizes the stacks of idle tasks to be consistent with other
> tasks on 32-bit.

It might be nice to stick a ud2 or 1: hlt; jmp 1b or similar
afterwards to make it clear that initial_code can't return.

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

#1462875 — Re: [PATCH v3 16/51] x86/32: put real return address on stack in entry code

On Sun, Aug 14, 2016 at 12:31:47AM -0700, Andy Lutomirski wrote:
> On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > This standardizes the stacks of idle tasks to be consistent with other
> > tasks on 32-bit.
> 
> It might be nice to stick a ud2 or 1: hlt; jmp 1b or similar
> afterwards to make it clear that initial_code can't return.

Yeah, I'll do something like that.

-- 
Josh

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

#1463055 — Re: [PATCH v3 16/51] x86/32: put real return address on stack in entry code

On 08/15/16 08:09, Josh Poimboeuf wrote:
> On Sun, Aug 14, 2016 at 12:31:47AM -0700, Andy Lutomirski wrote:
>> On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>>> This standardizes the stacks of idle tasks to be consistent with other
>>> tasks on 32-bit.
>>
>> It might be nice to stick a ud2 or 1: hlt; jmp 1b or similar
>> afterwards to make it clear that initial_code can't return.
> 
> Yeah, I'll do something like that.
> 

"Standardizing the stack" how?  A zero on the stack terminates the stack
trace.

	-hpa

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

#1463090 — Re: [PATCH v3 16/51] x86/32: put real return address on stack in entry code

On Mon, Aug 15, 2016 at 11:04:42AM -0700, H. Peter Anvin wrote:
> On 08/15/16 08:09, Josh Poimboeuf wrote:
> > On Sun, Aug 14, 2016 at 12:31:47AM -0700, Andy Lutomirski wrote:
> >> On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >>> This standardizes the stacks of idle tasks to be consistent with other
> >>> tasks on 32-bit.
> >>
> >> It might be nice to stick a ud2 or 1: hlt; jmp 1b or similar
> >> afterwards to make it clear that initial_code can't return.
> > 
> > Yeah, I'll do something like that.
> > 
> 
> "Standardizing the stack" how?  A zero on the stack terminates the stack
> trace.

Instead of zero, user tasks have a real return address at that spot.
This makes idle tasks consistent with that, so we have a well defined
"end of stack".  Also it makes the stack trace more useful since it
shows what entry code was involved in calling into C.

-- 
Josh

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

#1463122 — Re: [PATCH v3 16/51] x86/32: put real return address on stack in entry code

On 08/15/16 11:25, Josh Poimboeuf wrote:
> On Mon, Aug 15, 2016 at 11:04:42AM -0700, H. Peter Anvin wrote:
>> On 08/15/16 08:09, Josh Poimboeuf wrote:
>>> On Sun, Aug 14, 2016 at 12:31:47AM -0700, Andy Lutomirski wrote:
>>>> On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>>>>> This standardizes the stacks of idle tasks to be consistent with other
>>>>> tasks on 32-bit.
>>>>
>>>> It might be nice to stick a ud2 or 1: hlt; jmp 1b or similar
>>>> afterwards to make it clear that initial_code can't return.
>>>
>>> Yeah, I'll do something like that.
>>>
>>
>> "Standardizing the stack" how?  A zero on the stack terminates the stack
>> trace.
> 
> Instead of zero, user tasks have a real return address at that spot.
> This makes idle tasks consistent with that, so we have a well defined
> "end of stack".  Also it makes the stack trace more useful since it
> shows what entry code was involved in calling into C.
> 

So how is the stack terminated, and does things like kdb and kgdb need
modifications?  Or is there now a stack termination above the struct
pt_regs?

	-hpa

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

#1463146 — Re: [PATCH v3 16/51] x86/32: put real return address on stack in entry code

On Mon, Aug 15, 2016 at 12:22:33PM -0700, H. Peter Anvin wrote:
> On 08/15/16 11:25, Josh Poimboeuf wrote:
> > On Mon, Aug 15, 2016 at 11:04:42AM -0700, H. Peter Anvin wrote:
> >> On 08/15/16 08:09, Josh Poimboeuf wrote:
> >>> On Sun, Aug 14, 2016 at 12:31:47AM -0700, Andy Lutomirski wrote:
> >>>> On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >>>>> This standardizes the stacks of idle tasks to be consistent with other
> >>>>> tasks on 32-bit.
> >>>>
> >>>> It might be nice to stick a ud2 or 1: hlt; jmp 1b or similar
> >>>> afterwards to make it clear that initial_code can't return.
> >>>
> >>> Yeah, I'll do something like that.
> >>>
> >>
> >> "Standardizing the stack" how?  A zero on the stack terminates the stack
> >> trace.
> > 
> > Instead of zero, user tasks have a real return address at that spot.
> > This makes idle tasks consistent with that, so we have a well defined
> > "end of stack".  Also it makes the stack trace more useful since it
> > shows what entry code was involved in calling into C.
> > 
> 
> So how is the stack terminated, and does things like kdb and kgdb need
> modifications?  Or is there now a stack termination above the struct
> pt_regs?

Even in today's code, there's no real "terminator".   The unwinder just
stops when it leaves the stack bounds.  See print_context_stack() in
mainline.

-- 
Josh

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

#1461238 — [PATCH v3 18/51] x86/entry/head/32: use local labels

Add the local label prefix to all non-function named labels in head_32.S
and entry_32.S.  In addition to decluttering the symbol table, it also
causes stack traces to be more sensible.  For example, the last reported
function in the idle task stack trace is now startup_32_smp() instead of
is486().

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/entry/entry_32.S | 57 ++++++++++++++++++++++++-----------------------
 arch/x86/kernel/head_32.S | 32 +++++++++++++-------------
 2 files changed, 45 insertions(+), 44 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 0b56666..df4e045 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -64,7 +64,7 @@
 # define preempt_stop(clobbers)	DISABLE_INTERRUPTS(clobbers); TRACE_IRQS_OFF
 #else
 # define preempt_stop(clobbers)
-# define resume_kernel		restore_all
+# define resume_kernel		.Lrestore_all
 #endif
 
 .macro TRACE_IRQS_IRET
@@ -212,7 +212,7 @@ ENTRY(ret_from_fork)
 	/* When we fork, we trace the syscall return in the child, too. */
 	movl    %esp, %eax
 	call    syscall_return_slowpath
-	jmp     restore_all
+	jmp     .Lrestore_all
 END(ret_from_fork)
 
 ENTRY(ret_from_kernel_thread)
@@ -230,7 +230,7 @@ ENTRY(ret_from_kernel_thread)
 	 */
 	movl    %esp, %eax
 	call    syscall_return_slowpath
-	jmp     restore_all
+	jmp     .Lrestore_all
 ENDPROC(ret_from_kernel_thread)
 
 /*
@@ -264,19 +264,19 @@ ENTRY(resume_userspace)
 	TRACE_IRQS_OFF
 	movl	%esp, %eax
 	call	prepare_exit_to_usermode
-	jmp	restore_all
+	jmp	.Lrestore_all
 END(ret_from_exception)
 
 #ifdef CONFIG_PREEMPT
 ENTRY(resume_kernel)
 	DISABLE_INTERRUPTS(CLBR_ANY)
-need_resched:
+.Lneed_resched:
 	cmpl	$0, PER_CPU_VAR(__preempt_count)
-	jnz	restore_all
+	jnz	.Lrestore_all
 	testl	$X86_EFLAGS_IF, PT_EFLAGS(%esp)	# interrupts off (exception path) ?
-	jz	restore_all
+	jz	.Lrestore_all
 	call	preempt_schedule_irq
-	jmp	need_resched
+	jmp	.Lneed_resched
 END(resume_kernel)
 #endif
 
@@ -297,7 +297,7 @@ GLOBAL(__begin_SYSENTER_singlestep_region)
  */
 ENTRY(xen_sysenter_target)
 	addl	$5*4, %esp			/* remove xen-provided frame */
-	jmp	sysenter_past_esp
+	jmp	.Lsysenter_past_esp
 #endif
 
 /*
@@ -334,7 +334,7 @@ ENTRY(xen_sysenter_target)
  */
 ENTRY(entry_SYSENTER_32)
 	movl	TSS_sysenter_sp0(%esp), %esp
-sysenter_past_esp:
+.Lsysenter_past_esp:
 	pushl	$__USER_DS		/* pt_regs->ss */
 	pushl	%ebp			/* pt_regs->sp (stashed in bp) */
 	pushfl				/* pt_regs->flags (except IF = 0) */
@@ -465,11 +465,11 @@ ENTRY(entry_INT80_32)
 	call	do_int80_syscall_32
 .Lsyscall_32_done:
 
-restore_all:
+.Lrestore_all:
 	TRACE_IRQS_IRET
-restore_all_notrace:
+.Lrestore_all_notrace:
 #ifdef CONFIG_X86_ESPFIX32
-	ALTERNATIVE	"jmp restore_nocheck", "", X86_BUG_ESPFIX
+	ALTERNATIVE	"jmp .Lrestore_nocheck", "", X86_BUG_ESPFIX
 
 	movl	PT_EFLAGS(%esp), %eax		# mix EFLAGS, SS and CS
 	/*
@@ -481,22 +481,23 @@ restore_all_notrace:
 	movb	PT_CS(%esp), %al
 	andl	$(X86_EFLAGS_VM | (SEGMENT_TI_MASK << 8) | SEGMENT_RPL_MASK), %eax
 	cmpl	$((SEGMENT_LDT << 8) | USER_RPL), %eax
-	je ldt_ss				# returning to user-space with LDT SS
+	je .Lldt_ss				# returning to user-space with LDT SS
 #endif
-restore_nocheck:
+.Lrestore_nocheck:
 	RESTORE_REGS 4				# skip orig_eax/error_code
-irq_return:
+.Lirq_return:
 	INTERRUPT_RETURN
+
 .section .fixup, "ax"
 ENTRY(iret_exc	)
 	pushl	$0				# no error code
 	pushl	$do_iret_error
 	jmp	error_code
 .previous
-	_ASM_EXTABLE(irq_return, iret_exc)
+	_ASM_EXTABLE(.Lirq_return, iret_exc)
 
 #ifdef CONFIG_X86_ESPFIX32
-ldt_ss:
+.Lldt_ss:
 /*
  * Setup and switch to ESPFIX stack
  *
@@ -525,7 +526,7 @@ ldt_ss:
 	 */
 	DISABLE_INTERRUPTS(CLBR_EAX)
 	lss	(%esp), %esp			/* switch to espfix segment */
-	jmp	restore_nocheck
+	jmp	.Lrestore_nocheck
 #endif
 ENDPROC(entry_INT80_32)
 
@@ -845,7 +846,7 @@ ftrace_call:
 	popl	%edx
 	popl	%ecx
 	popl	%eax
-ftrace_ret:
+.Lftrace_ret:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call
 ftrace_graph_call:
@@ -915,7 +916,7 @@ GLOBAL(ftrace_regs_call)
 	popl	%gs
 	addl	$8, %esp			/* Skip orig_ax and ip */
 	popf					/* Pop flags at end (no addl to corrupt flags) */
-	jmp	ftrace_ret
+	jmp	.Lftrace_ret
 
 	popf
 	jmp	ftrace_stub
@@ -926,7 +927,7 @@ ENTRY(mcount)
 	jb	ftrace_stub			/* Paging not enabled yet? */
 
 	cmpl	$ftrace_stub, ftrace_trace_function
-	jnz	trace
+	jnz	.Ltrace
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	cmpl	$ftrace_stub, ftrace_graph_return
 	jnz	ftrace_graph_caller
@@ -939,7 +940,7 @@ ftrace_stub:
 	ret
 
 	/* taken from glibc */
-trace:
+.Ltrace:
 	pushl	%eax
 	pushl	%ecx
 	pushl	%edx
@@ -1078,7 +1079,7 @@ ENTRY(nmi)
 	movl	%ss, %eax
 	cmpw	$__ESPFIX_SS, %ax
 	popl	%eax
-	je	nmi_espfix_stack
+	je	.Lnmi_espfix_stack
 #endif
 
 	pushl	%eax				# pt_regs->orig_ax
@@ -1094,7 +1095,7 @@ ENTRY(nmi)
 
 	/* Not on SYSENTER stack. */
 	call	do_nmi
-	jmp	restore_all_notrace
+	jmp	.Lrestore_all_notrace
 
 .Lnmi_from_sysenter_stack:
 	/*
@@ -1105,10 +1106,10 @@ ENTRY(nmi)
 	movl	PER_CPU_VAR(cpu_current_top_of_stack), %esp
 	call	do_nmi
 	movl	%ebp, %esp
-	jmp	restore_all_notrace
+	jmp	.Lrestore_all_notrace
 
 #ifdef CONFIG_X86_ESPFIX32
-nmi_espfix_stack:
+.Lnmi_espfix_stack:
 	/*
 	 * create the pointer to lss back
 	 */
@@ -1126,7 +1127,7 @@ nmi_espfix_stack:
 	call	do_nmi
 	RESTORE_REGS
 	lss	12+4(%esp), %esp		# back to espfix stack
-	jmp	irq_return
+	jmp	.Lirq_return
 #endif
 END(nmi)
 
diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index 6fc4f1d..53202d7 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -249,19 +249,19 @@ page_pde_offset = (__PAGE_OFFSET >> 20);
 #ifdef CONFIG_PARAVIRT
 	/* This is can only trip for a broken bootloader... */
 	cmpw $0x207, pa(boot_params + BP_version)
-	jb default_entry
+	jb .Ldefault_entry
 
 	/* Paravirt-compatible boot parameters.  Look to see what architecture
 		we're booting under. */
 	movl pa(boot_params + BP_hardware_subarch), %eax
 	cmpl $num_subarch_entries, %eax
-	jae bad_subarch
+	jae .Lbad_subarch
 
 	movl pa(subarch_entries)(,%eax,4), %eax
 	subl $__PAGE_OFFSET, %eax
 	jmp *%eax
 
-bad_subarch:
+.Lbad_subarch:
 WEAK(lguest_entry)
 WEAK(xen_entry)
 	/* Unknown implementation; there's really
@@ -271,14 +271,14 @@ WEAK(xen_entry)
 	__INITDATA
 
 subarch_entries:
-	.long default_entry		/* normal x86/PC */
+	.long .Ldefault_entry		/* normal x86/PC */
 	.long lguest_entry		/* lguest hypervisor */
 	.long xen_entry			/* Xen hypervisor */
-	.long default_entry		/* Moorestown MID */
+	.long .Ldefault_entry		/* Moorestown MID */
 num_subarch_entries = (. - subarch_entries) / 4
 .previous
 #else
-	jmp default_entry
+	jmp .Ldefault_entry
 #endif /* CONFIG_PARAVIRT */
 
 #ifdef CONFIG_HOTPLUG_CPU
@@ -318,7 +318,7 @@ ENTRY(startup_32_smp)
 	call load_ucode_ap
 #endif
 
-default_entry:
+.Ldefault_entry:
 #define CR0_STATE	(X86_CR0_PE | X86_CR0_MP | X86_CR0_ET | \
 			 X86_CR0_NE | X86_CR0_WP | X86_CR0_AM | \
 			 X86_CR0_PG)
@@ -348,7 +348,7 @@ default_entry:
 	pushfl
 	popl %eax			# get EFLAGS
 	testl $X86_EFLAGS_ID,%eax	# did EFLAGS.ID remained set?
-	jz enable_paging		# hw disallowed setting of ID bit
+	jz .Lenable_paging		# hw disallowed setting of ID bit
 					# which means no CPUID and no CR4
 
 	xorl %eax,%eax
@@ -358,13 +358,13 @@ default_entry:
 	movl $1,%eax
 	cpuid
 	andl $~1,%edx			# Ignore CPUID.FPU
-	jz enable_paging		# No flags or only CPUID.FPU = no CR4
+	jz .Lenable_paging		# No flags or only CPUID.FPU = no CR4
 
 	movl pa(mmu_cr4_features),%eax
 	movl %eax,%cr4
 
 	testb $X86_CR4_PAE, %al		# check if PAE is enabled
-	jz enable_paging
+	jz .Lenable_paging
 
 	/* Check if extended functions are implemented */
 	movl $0x80000000, %eax
@@ -372,7 +372,7 @@ default_entry:
 	/* Value must be in the range 0x80000001 to 0x8000ffff */
 	subl $0x80000001, %eax
 	cmpl $(0x8000ffff-0x80000001), %eax
-	ja enable_paging
+	ja .Lenable_paging
 
 	/* Clear bogus XD_DISABLE bits */
 	call verify_cpu
@@ -381,7 +381,7 @@ default_entry:
 	cpuid
 	/* Execute Disable bit supported? */
 	btl $(X86_FEATURE_NX & 31), %edx
-	jnc enable_paging
+	jnc .Lenable_paging
 
 	/* Setup EFER (Extended Feature Enable Register) */
 	movl $MSR_EFER, %ecx
@@ -391,7 +391,7 @@ default_entry:
 	/* Make changes effective */
 	wrmsr
 
-enable_paging:
+.Lenable_paging:
 
 /*
  * Enable paging
@@ -420,7 +420,7 @@ enable_paging:
  */
 	movb $4,X86			# at least 486
 	cmpl $-1,X86_CPUID
-	je is486
+	je .Lis486
 
 	/* get vendor info */
 	xorl %eax,%eax			# call CPUID with 0 -> return vendor ID
@@ -431,7 +431,7 @@ enable_paging:
 	movl %ecx,X86_VENDOR_ID+8	# last 4 chars
 
 	orl %eax,%eax			# do we have processor info as well?
-	je is486
+	je .Lis486
 
 	movl $1,%eax		# Use the CPUID instruction to get CPU type
 	cpuid
@@ -445,7 +445,7 @@ enable_paging:
 	movb %cl,X86_MASK
 	movl %edx,X86_CAPABILITY
 
-is486:
+.Lis486:
 	movl $0x50022,%ecx	# set AM, WP, NE and MP
 	movl %cr0,%eax
 	andl $0x80000011,%eax	# Save PG,PE,ET
-- 
2.7.4

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

#1461239 — [PATCH v3 30/51] x86/dumpstack/ftrace: don't print unreliable addresses in print_context_stack_bp()

When function graph tracing is enabled, print_context_stack_bp() can
report return_to_handler() as an unreliable address, which is confusing
and misleading: return_to_handler() is really only useful as a hint for
debugging, whereas print_context_stack_bp() users only care about the
actual 'reliable' call path.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/dumpstack.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 33f2899..c6c6c39 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -127,8 +127,6 @@ print_context_stack_bp(struct task_struct *task,
 			break;
 
 		real_addr = ftrace_graph_ret_addr(task, graph, addr, retp);
-		if (real_addr != addr && ops->address(data, addr, 0))
-			break;
 		if (ops->address(data, real_addr, 1))
 			break;
 
-- 
2.7.4

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

#1461240 — [PATCH v3 42/51] x86/unwind: create stack frames for saved syscall registers

The entry code doesn't encode pt_regs for syscalls.  But they're always
at the same location, so we can add a manual check for them.

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

diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index e02acec..d2480a3 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -24,6 +24,14 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
 }
 EXPORT_SYMBOL_GPL(unwind_get_return_address);
 
+static bool is_last_task_frame(struct unwind_state *state)
+{
+	unsigned long bp = (unsigned long)state->bp;
+	unsigned long regs = (unsigned long)task_pt_regs(state->task);
+
+	return bp == regs - FRAME_HEADER_SIZE;
+}
+
 /*
  * This determines if the frame pointer actually contains an encoded pointer to
  * pt_regs on the stack.  See ENCODE_FRAME_POINTER.
@@ -77,6 +85,33 @@ bool unwind_next_frame(struct unwind_state *state)
 	if (state->regs && user_mode(state->regs))
 		goto the_end;
 
+	if (is_last_task_frame(state)) {
+		regs = task_pt_regs(state->task);
+
+		/*
+		 * kthreads (other than the boot CPU's idle thread) have some
+		 * partial regs at the end of their stack which were placed
+		 * there by copy_thread_tls().  But the regs don't have any
+		 * useful information, so we can skip them.
+		 *
+		 * This user_mode() check is slightly broader than a PF_KTHREAD
+		 * check because it also catches the awkward situation where a
+		 * newly forked kthread transitions into a user task by calling
+		 * do_execve(), which eventually clears PF_KTHREAD.
+		 */
+		if (!user_mode(regs))
+			goto the_end;
+
+		/*
+		 * We're almost at the end, but not quite: there's still the
+		 * syscall regs frame.  Entry code doesn't encode the regs
+		 * pointer for syscalls, so we have to set it manually.
+		 */
+		state->regs = regs;
+		state->bp = NULL;
+		return true;
+	}
+
 	/* get the next frame pointer */
 	if (state->regs)
 		next_bp = (unsigned long *)state->regs->bp;
-- 
2.7.4

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

#1461702 — Re: [PATCH v3 42/51] x86/unwind: create stack frames for saved syscall registers

On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> The entry code doesn't encode pt_regs for syscalls.  But they're always
> at the same location, so we can add a manual check for them.

At first I thought these would be useless (they're the *user* state
and aren't directly relevant to the kernel), but then I realized that
they could be extremely valuable: they contain the syscall args.  Do
they display in OOPS dumps?  It might be nice to add orig_ax to the
regs display too so we get the syscall nr as well.

--Andy

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

#1462993 — Re: [PATCH v3 42/51] x86/unwind: create stack frames for saved syscall registers

On Sun, Aug 14, 2016 at 01:23:11AM -0700, Andy Lutomirski wrote:
> On Fri, Aug 12, 2016 at 7:29 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > The entry code doesn't encode pt_regs for syscalls.  But they're always
> > at the same location, so we can add a manual check for them.
> 
> At first I thought these would be useless (they're the *user* state
> and aren't directly relevant to the kernel), but then I realized that
> they could be extremely valuable: they contain the syscall args.

Yeah, that's what I was thinking.  I'll add that justification to the
patch header.

> Do they display in OOPS dumps?

Yes.

> It might be nice to add orig_ax to the regs display too so we get the
> syscall nr as well.

Hm, good idea.  I'm surprised orig_ax isn't already being shown since it
could be useful for exceptions and irqs too.

-- 
Josh

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

#1461241 — [PATCH v3 28/51] ftrace/x86: implement HAVE_FUNCTION_GRAPH_RET_ADDR_PTR

This allows the use of the more reliable version of
ftrace_graph_ret_addr() so we no longer have to worry about the unwinder
getting out of sync with the function graph ret_stack index, which can
happen if the unwinder skips any frames before calling
ftrace_graph_ret_addr().

This fixes this issue (and several others like it):

  Before:
  $ cat /proc/self/stack
  [<ffffffff810489a2>] save_stack_trace_tsk+0x22/0x40
  [<ffffffff81311a89>] proc_pid_stack+0xb9/0x110
  [<ffffffff813127c4>] proc_single_show+0x54/0x80
  [<ffffffff812be088>] seq_read+0x108/0x3e0
  [<ffffffff812923d7>] __vfs_read+0x37/0x140
  [<ffffffff812929d9>] vfs_read+0x99/0x140
  [<ffffffff81293f28>] SyS_read+0x58/0xc0
  [<ffffffff818af97c>] entry_SYSCALL_64_fastpath+0x1f/0xbd
  [<ffffffffffffffff>] 0xffffffffffffffff

  After:
  $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
  $ cat /proc/self/stack
  [<ffffffff818b2428>] ? return_to_handler+0x0/0x27
  [<ffffffff810394cc>] print_context_stack+0xfc/0x100
  [<ffffffff818b2428>] ? return_to_handler+0x0/0x27
  [<ffffffff8103891b>] dump_trace+0x12b/0x350
  [<ffffffff818b2428>] ? return_to_handler+0x0/0x27
  [<ffffffff810489a2>] save_stack_trace_tsk+0x22/0x40
  [<ffffffff818b2428>] ? return_to_handler+0x0/0x27
  [<ffffffff81311a89>] proc_pid_stack+0xb9/0x110
  [<ffffffff818b2428>] ? return_to_handler+0x0/0x27
  [<ffffffff813127c4>] proc_single_show+0x54/0x80
  [<ffffffff818b2428>] ? return_to_handler+0x0/0x27
  [<ffffffff812be088>] seq_read+0x108/0x3e0
  [<ffffffff818b2428>] ? return_to_handler+0x0/0x27
  [<ffffffff812923d7>] __vfs_read+0x37/0x140
  [<ffffffff818b2428>] ? return_to_handler+0x0/0x27
  [<ffffffff812929d9>] vfs_read+0x99/0x140
  [<ffffffffffffffff>] 0xffffffffffffffff

Enabling function graph tracing caused the stack trace to change: it was
offset by 2 frames because the unwinder started with an earlier stack
frame and got out of sync with the ret_stack index.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/ftrace.h | 2 ++
 arch/x86/kernel/ftrace.c      | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index 37f67cb..eccd0ac 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -14,6 +14,8 @@
 #define ARCH_SUPPORTS_FTRACE_OPS 1
 #endif
 
+#define HAVE_FUNCTION_GRAPH_RET_ADDR_PTR
+
 #ifndef __ASSEMBLY__
 extern void mcount(void);
 extern atomic_t modifying_ftrace_code;
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index ae3b1fb..8639bb2 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -1029,7 +1029,7 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 	}
 
 	if (ftrace_push_return_trace(old, self_addr, &trace.depth,
-				     frame_pointer, NULL) == -EBUSY) {
+				     frame_pointer, parent) == -EBUSY) {
 		*parent = old;
 		return;
 	}
-- 
2.7.4

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

#1461243 — [PATCH v3 31/51] x86/dumpstack: allow preemption in show_stack_log_lvl() and dump_trace()

show_stack_log_lvl() and dump_trace() are already preemption safe:

- If they're running in interrupt context, preemption is already
  disabled and the percpu irq stack pointers can be trusted.

- If they're running with preemption enabled, they must be running on
  the task stack anyway, so it doesn't matter if they're comparing the
  stack pointer against the percpu irq stack pointer from this CPU or
  another one: either way it won't match.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/dumpstack_32.c | 14 ++++++--------
 arch/x86/kernel/dumpstack_64.c | 26 +++++++++-----------------
 2 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index c533b8b..b07d5c9 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -24,16 +24,16 @@ static void *is_irq_stack(void *p, void *irq)
 }
 
 
-static void *is_hardirq_stack(unsigned long *stack, int cpu)
+static void *is_hardirq_stack(unsigned long *stack)
 {
-	void *irq = per_cpu(hardirq_stack, cpu);
+	void *irq = this_cpu_read(hardirq_stack);
 
 	return is_irq_stack(stack, irq);
 }
 
-static void *is_softirq_stack(unsigned long *stack, int cpu)
+static void *is_softirq_stack(unsigned long *stack);
 {
-	void *irq = per_cpu(softirq_stack, cpu);
+	void *irq = this_cpu_read(softirq_stack);
 
 	return is_irq_stack(stack, irq);
 }
@@ -42,7 +42,6 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 		unsigned long *stack, unsigned long bp,
 		const struct stacktrace_ops *ops, void *data)
 {
-	const unsigned cpu = get_cpu();
 	int graph = 0;
 	u32 *prev_esp;
 
@@ -53,9 +52,9 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 	for (;;) {
 		void *end_stack;
 
-		end_stack = is_hardirq_stack(stack, cpu);
+		end_stack = is_hardirq_stack(stack);
 		if (!end_stack)
-			end_stack = is_softirq_stack(stack, cpu);
+			end_stack = is_softirq_stack(stack);
 
 		bp = ops->walk_stack(task, stack, bp, ops, data,
 				     end_stack, &graph);
@@ -74,7 +73,6 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 			break;
 		touch_nmi_watchdog();
 	}
-	put_cpu();
 }
 EXPORT_SYMBOL(dump_trace);
 
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 491f2fd..f1b843a 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -31,8 +31,8 @@ static char x86_stack_ids[][8] = {
 #endif
 };
 
-static unsigned long *in_exception_stack(unsigned cpu, unsigned long stack,
-					 unsigned *usedp, char **idp)
+static unsigned long *in_exception_stack(unsigned long stack, unsigned *usedp,
+					 char **idp)
 {
 	unsigned k;
 
@@ -41,7 +41,7 @@ static unsigned long *in_exception_stack(unsigned cpu, unsigned long stack,
 	 * 'stack' is in one of them:
 	 */
 	for (k = 0; k < N_EXCEPTION_STACKS; k++) {
-		unsigned long end = per_cpu(orig_ist, cpu).ist[k];
+		unsigned long end = raw_cpu_ptr(&orig_ist)->ist[k];
 		/*
 		 * Is 'stack' above this exception frame's end?
 		 * If yes then skip to the next frame.
@@ -111,7 +111,7 @@ enum stack_type {
 };
 
 static enum stack_type
-analyze_stack(int cpu, struct task_struct *task, unsigned long *stack,
+analyze_stack(struct task_struct *task, unsigned long *stack,
 	      unsigned long **stack_end, unsigned long *irq_stack,
 	      unsigned *used, char **id)
 {
@@ -121,8 +121,7 @@ analyze_stack(int cpu, struct task_struct *task, unsigned long *stack,
 	if ((unsigned long)task_stack_page(task) == addr)
 		return STACK_IS_NORMAL;
 
-	*stack_end = in_exception_stack(cpu, (unsigned long)stack,
-					used, id);
+	*stack_end = in_exception_stack((unsigned long)stack, used, id);
 	if (*stack_end)
 		return STACK_IS_EXCEPTION;
 
@@ -149,8 +148,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 		unsigned long *stack, unsigned long bp,
 		const struct stacktrace_ops *ops, void *data)
 {
-	const unsigned cpu = get_cpu();
-	unsigned long *irq_stack = (unsigned long *)per_cpu(irq_stack_ptr, cpu);
+	unsigned long *irq_stack = (unsigned long *)this_cpu_read(irq_stack_ptr);
 	unsigned used = 0;
 	int graph = 0;
 	int done = 0;
@@ -169,8 +167,8 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 		enum stack_type stype;
 		char *id;
 
-		stype = analyze_stack(cpu, task, stack, &stack_end,
-				      irq_stack, &used, &id);
+		stype = analyze_stack(task, stack, &stack_end, irq_stack, &used,
+				      &id);
 
 		/* Default finish unless specified to continue */
 		done = 1;
@@ -225,7 +223,6 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 	 * This handles the process stack:
 	 */
 	bp = ops->walk_stack(task, stack, bp, ops, data, NULL, &graph);
-	put_cpu();
 }
 EXPORT_SYMBOL(dump_trace);
 
@@ -236,13 +233,9 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 	unsigned long *irq_stack_end;
 	unsigned long *irq_stack;
 	unsigned long *stack;
-	int cpu;
 	int i;
 
-	preempt_disable();
-	cpu = smp_processor_id();
-
-	irq_stack_end = (unsigned long *)(per_cpu(irq_stack_ptr, cpu));
+	irq_stack_end = (unsigned long *)this_cpu_read(irq_stack_ptr);
 	irq_stack     = irq_stack_end - (IRQ_USABLE_STACK_SIZE / sizeof(long));
 
 	sp = sp ? : get_stack_pointer(task, regs);
@@ -274,7 +267,6 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 		stack++;
 		touch_nmi_watchdog();
 	}
-	preempt_enable();
 
 	pr_cont("\n");
 	show_trace_log_lvl(task, regs, sp, bp, log_lvl);
-- 
2.7.4

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

#1461706 — Re: [PATCH v3 31/51] x86/dumpstack: allow preemption in show_stack_log_lvl() and dump_trace()

On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> show_stack_log_lvl() and dump_trace() are already preemption safe:
>
> - If they're running in interrupt context, preemption is already
>   disabled and the percpu irq stack pointers can be trusted.

I agree with the patch, but I have a minor nitpick about the
description.  The "irq stack" is an actual thing, but the relevant
stacks here aren't just the irq stack: they're the irq stack, the IST
stacks (on 64-bit), and the NMI stack (on 32-bit).  The same logic
applies, though.

--Andy

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

#1462931 — Re: [PATCH v3 31/51] x86/dumpstack: allow preemption in show_stack_log_lvl() and dump_trace()

On Sun, Aug 14, 2016 at 12:45:35AM -0700, Andy Lutomirski wrote:
> On Fri, Aug 12, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > show_stack_log_lvl() and dump_trace() are already preemption safe:
> >
> > - If they're running in interrupt context, preemption is already
> >   disabled and the percpu irq stack pointers can be trusted.
> 
> I agree with the patch, but I have a minor nitpick about the
> description.  The "irq stack" is an actual thing, but the relevant
> stacks here aren't just the irq stack: they're the irq stack, the IST
> stacks (on 64-bit), and the NMI stack (on 32-bit).  The same logic
> applies, though.

Yeah.  Maybe I'll just remove the "irq" qualifier and instead call them
"percpu stack pointers".

-- 
Josh

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

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

Back to top | Article view | linux.kernel

csiph-web