[PATCH 0/2 v2] tracing/arm: Fix the stack tracer when LR is saved after local storage

[PATCH 0/2 v2] tracing/arm: Fix the stack tracer when LR is saved after local storage

[Steven Rostedt]

As arm64 saves the link register after a function's local variables are
stored, it causes the max stack tracer to be off by one in its output
of which function has the bloated stack frame.

The first patch fixes this by creating a ARCH_RET_ADDR_BEFORE_LOCAL_VARS
define that an achitecture (arm64) may set in asm/ftrace.h, and this
will cause the stack tracer to make the shift.

As it has been proven that the stack tracer isn't the most trivial
algorithm to understand by staring at the code, the second patch adds
comments to the code to explain the algorithm with and without the
ARCH_RET_ADDR_BEFORE_LOCAL_VARS.

Hmm, should this be sent to stable (and for inclusion now?)

-- Steve

Changes since v1:

 - Fixed wrong value in stack_trace_index[] array in comment

 - Added a comment about gcc currently saves the LR after local variables,
   but there's no guarantee that it will be like that in the future.
   (Notified of this by Mark Rutland)

Steven Rostedt (VMware) (2):
      tracing/arm64: Have max stack tracer handle the case of return address after data
      tracing: Document the stack trace algorithm in the comments

----
 arch/arm64/include/asm/ftrace.h |  13 +++++
 kernel/trace/trace_stack.c      | 112 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+)

[PATCH 1/2 v2] tracing/arm64: Have max stack tracer handle the case of return address after data

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Most archs (well at least x86) store the function call return address on the
stack before storing the local variables for the function. The max stack
tracer depends on this in its algorithm to display the stack size of each
function it finds in the back trace.

Some archs (arm64), may store the return address (from its link register)
just before calling a nested function. There's no reason to save the link
register on leaf functions, as it wont be updated. This breaks the algorithm
of the max stack tracer.

Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set
if it stores the return address (link register) after it stores the
function's local variables, and have the stack trace shift the values of the
mapped stack size to the appropriate functions.

Link: 20190802094103.163576-1-jiping.ma2@windriver.com

Reported-by: Jiping Ma <jiping.ma2@windriver.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/arm64/include/asm/ftrace.h | 13 +++++++++++++
 kernel/trace/trace_stack.c      | 14 ++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 5ab5200b2bdc..961e98618db4 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -14,6 +14,19 @@
 #define MCOUNT_ADDR		((unsigned long)_mcount)
 #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
 
+/*
+ * Currently, gcc tends to save the link register after the local variables
+ * on the stack. This causes the max stack tracer to report the function
+ * frame sizes for the wrong functions. By defining
+ * ARCH_RET_ADDR_AFTER_LOCAL_VARS, it will tell the stack tracer to expect
+ * to find the return address on the stack after the local variables have
+ * been set up.
+ *
+ * Note, this may change in the future, and we will need to deal with that
+ * if it were to happen.
+ */
+#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1
+
 #ifndef __ASSEMBLY__
 #include <linux/compat.h>
 
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 5d16f73898db..40e4a88eea8f 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -158,6 +158,20 @@ static void check_stack(unsigned long ip, unsigned long *stack)
 			i++;
 	}
 
+#ifdef ARCH_RET_ADDR_AFTER_LOCAL_VARS
+	/*
+	 * Some archs will store the link register before calling
+	 * nested functions. This means the saved return address
+	 * comes after the local storage, and we need to shift
+	 * for that.
+	 */
+	if (x > 1) {
+		memmove(&stack_trace_index[0], &stack_trace_index[1],
+			sizeof(stack_trace_index[0]) * (x - 1));
+		x--;
+	}
+#endif
+
 	stack_trace_nr_entries = x;
 
 	if (task_stack_end_corrupted(current)) {
-- 
2.20.1

Re: [PATCH 1/2 v2] tracing/arm64: Have max stack tracer handle the case of return address after data

[Steven Rostedt]

[ I should have added Mark as Cc ]

Dear ARM64 folks,

Are you OK with this patch set?

If so, please ACK.

Should it be marked for stable?

Hmm, I'm starting to think not.

-- Steve


On Wed, 07 Aug 2019 13:28:27 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Most archs (well at least x86) store the function call return address on the
> stack before storing the local variables for the function. The max stack
> tracer depends on this in its algorithm to display the stack size of each
> function it finds in the back trace.
> 
> Some archs (arm64), may store the return address (from its link register)
> just before calling a nested function. There's no reason to save the link
> register on leaf functions, as it wont be updated. This breaks the algorithm
> of the max stack tracer.
> 
> Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set
> if it stores the return address (link register) after it stores the
> function's local variables, and have the stack trace shift the values of the
> mapped stack size to the appropriate functions.
> 
> Link: 20190802094103.163576-1-jiping.ma2@windriver.com
> 
> Reported-by: Jiping Ma <jiping.ma2@windriver.com>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  arch/arm64/include/asm/ftrace.h | 13 +++++++++++++
>  kernel/trace/trace_stack.c      | 14 ++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 5ab5200b2bdc..961e98618db4 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -14,6 +14,19 @@
>  #define MCOUNT_ADDR		((unsigned long)_mcount)
>  #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
>  
> +/*
> + * Currently, gcc tends to save the link register after the local variables
> + * on the stack. This causes the max stack tracer to report the function
> + * frame sizes for the wrong functions. By defining
> + * ARCH_RET_ADDR_AFTER_LOCAL_VARS, it will tell the stack tracer to expect
> + * to find the return address on the stack after the local variables have
> + * been set up.
> + *
> + * Note, this may change in the future, and we will need to deal with that
> + * if it were to happen.
> + */
> +#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1
> +
>  #ifndef __ASSEMBLY__
>  #include <linux/compat.h>
>  
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index 5d16f73898db..40e4a88eea8f 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -158,6 +158,20 @@ static void check_stack(unsigned long ip, unsigned long *stack)
>  			i++;
>  	}
>  
> +#ifdef ARCH_RET_ADDR_AFTER_LOCAL_VARS
> +	/*
> +	 * Some archs will store the link register before calling
> +	 * nested functions. This means the saved return address
> +	 * comes after the local storage, and we need to shift
> +	 * for that.
> +	 */
> +	if (x > 1) {
> +		memmove(&stack_trace_index[0], &stack_trace_index[1],
> +			sizeof(stack_trace_index[0]) * (x - 1));
> +		x--;
> +	}
> +#endif
> +
>  	stack_trace_nr_entries = x;
>  
>  	if (task_stack_end_corrupted(current)) {

Re: [PATCH 1/2 v2] tracing/arm64: Have max stack tracer handle the case of return address after data

[Will Deacon]

Hi Steve,

On Wed, Aug 07, 2019 at 01:28:27PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Most archs (well at least x86) store the function call return address on the
> stack before storing the local variables for the function. The max stack
> tracer depends on this in its algorithm to display the stack size of each
> function it finds in the back trace.
> 
> Some archs (arm64), may store the return address (from its link register)
> just before calling a nested function. There's no reason to save the link
> register on leaf functions, as it wont be updated. This breaks the algorithm
> of the max stack tracer.
> 
> Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set
> if it stores the return address (link register) after it stores the
> function's local variables, and have the stack trace shift the values of the
> mapped stack size to the appropriate functions.
> 
> Link: 20190802094103.163576-1-jiping.ma2@windriver.com
> 
> Reported-by: Jiping Ma <jiping.ma2@windriver.com>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  arch/arm64/include/asm/ftrace.h | 13 +++++++++++++
>  kernel/trace/trace_stack.c      | 14 ++++++++++++++
>  2 files changed, 27 insertions(+)

I agree with your later comment that this should NOT go to stable.

> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 5ab5200b2bdc..961e98618db4 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -14,6 +14,19 @@
>  #define MCOUNT_ADDR		((unsigned long)_mcount)
>  #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
>  
> +/*
> + * Currently, gcc tends to save the link register after the local variables
> + * on the stack. This causes the max stack tracer to report the function
> + * frame sizes for the wrong functions. By defining
> + * ARCH_RET_ADDR_AFTER_LOCAL_VARS, it will tell the stack tracer to expect
> + * to find the return address on the stack after the local variables have
> + * been set up.
> + *
> + * Note, this may change in the future, and we will need to deal with that
> + * if it were to happen.
> + */
> +#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1

I know it's long already, but prefixing this with FTRACE_ would be good so
that other code doesn't use it for anything. It's not the end of the world
if the ftrace stack usage statistics are wonky, but if people tried to use
this for crazy things like livepatching then we'd be in trouble.

Maybe FTRACE_ARCH_FRAME_AFTER_LOCALS, which is the same length as what
you currently have?

Will

Re: [PATCH 1/2 v2] tracing/arm64: Have max stack tracer handle the case of return address after data

[Steven Rostedt]

On Thu, 8 Aug 2019 17:28:26 +0100
Will Deacon <will@kernel.org> wrote:

> > + * Note, this may change in the future, and we will need to deal with that
> > + * if it were to happen.
> > + */
> > +#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1  
> 
> I know it's long already, but prefixing this with FTRACE_ would be good so
> that other code doesn't use it for anything. It's not the end of the world
> if the ftrace stack usage statistics are wonky, but if people tried to use
> this for crazy things like livepatching then we'd be in trouble.
> 
> Maybe FTRACE_ARCH_FRAME_AFTER_LOCALS, which is the same length as what
> you currently have?

Note, it would still need to be prefixed with "ARCH_" as that's the way
of showing arch specific defines.

We could make it more descriptive of what it will do and not the reason
for why it is done...


  ARCH_FTRACE_SHIFT_STACK_TRACER

?

-- Steve

Re: [PATCH 1/2 v2] tracing/arm64: Have max stack tracer handle the case of return address after data

[Will Deacon]

On Thu, Aug 08, 2019 at 12:36:32PM -0400, Steven Rostedt wrote:
> On Thu, 8 Aug 2019 17:28:26 +0100
> Will Deacon <will@kernel.org> wrote:
> 
> > > + * Note, this may change in the future, and we will need to deal with that
> > > + * if it were to happen.
> > > + */
> > > +#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1  
> > 
> > I know it's long already, but prefixing this with FTRACE_ would be good so
> > that other code doesn't use it for anything. It's not the end of the world
> > if the ftrace stack usage statistics are wonky, but if people tried to use
> > this for crazy things like livepatching then we'd be in trouble.
> > 
> > Maybe FTRACE_ARCH_FRAME_AFTER_LOCALS, which is the same length as what
> > you currently have?
> 
> Note, it would still need to be prefixed with "ARCH_" as that's the way
> of showing arch specific defines.
> 
> We could make it more descriptive of what it will do and not the reason
> for why it is done...
> 
> 
>   ARCH_FTRACE_SHIFT_STACK_TRACER

Acked-by: Will Deacon <will@kernel.org>

Thanks, Steve.

Will

Re: [PATCH 1/2 v2] tracing/arm64: Have max stack tracer handle the case of return address after data

[Steven Rostedt]

On Thu, 8 Aug 2019 18:11:53 +0100
Will Deacon <will@kernel.org> wrote:

> > We could make it more descriptive of what it will do and not the reason
> > for why it is done...
> > 
> > 
> >   ARCH_FTRACE_SHIFT_STACK_TRACER  
> 
> Acked-by: Will Deacon <will@kernel.org>

Thanks Will!

Here's the official patch.

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Most archs (well at least x86) store the function call return address on the
stack before storing the local variables for the function. The max stack
tracer depends on this in its algorithm to display the stack size of each
function it finds in the back trace.

Some archs (arm64), may store the return address (from its link register)
just before calling a nested function. There's no reason to save the link
register on leaf functions, as it wont be updated. This breaks the algorithm
of the max stack tracer.

Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set
if it stores the return address (link register) after it stores the
function's local variables, and have the stack trace shift the values of the
mapped stack size to the appropriate functions.

Link: 20190802094103.163576-1-jiping.ma2@windriver.com

Reported-by: Jiping Ma <jiping.ma2@windriver.com>
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/arm64/include/asm/ftrace.h | 13 +++++++++++++
 kernel/trace/trace_stack.c      | 14 ++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 5ab5200b2bdc..d48667b04c41 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -14,6 +14,19 @@
 #define MCOUNT_ADDR		((unsigned long)_mcount)
 #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
 
+/*
+ * Currently, gcc tends to save the link register after the local variables
+ * on the stack. This causes the max stack tracer to report the function
+ * frame sizes for the wrong functions. By defining
+ * ARCH_FTRACE_SHIFT_STACK_TRACER, it will tell the stack tracer to expect
+ * to find the return address on the stack after the local variables have
+ * been set up.
+ *
+ * Note, this may change in the future, and we will need to deal with that
+ * if it were to happen.
+ */
+#define ARCH_FTRACE_SHIFT_STACK_TRACER 1
+
 #ifndef __ASSEMBLY__
 #include <linux/compat.h>
 
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 5d16f73898db..642a850af81a 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -158,6 +158,20 @@ static void check_stack(unsigned long ip, unsigned long *stack)
 			i++;
 	}
 
+#ifdef ARCH_FTRACE_SHIFT_STACK_TRACER
+	/*
+	 * Some archs will store the link register before calling
+	 * nested functions. This means the saved return address
+	 * comes after the local storage, and we need to shift
+	 * for that.
+	 */
+	if (x > 1) {
+		memmove(&stack_trace_index[0], &stack_trace_index[1],
+			sizeof(stack_trace_index[0]) * (x - 1));
+		x--;
+	}
+#endif
+
 	stack_trace_nr_entries = x;
 
 	if (task_stack_end_corrupted(current)) {
-- 
2.20.1

Re: [PATCH 1/2 v2] tracing/arm64: Have max stack tracer handle the case of return address after data

[Jiping Ma]

On 2019年08月09日 01:24, Steven Rostedt wrote:
> On Thu, 8 Aug 2019 18:11:53 +0100
> Will Deacon <will@kernel.org> wrote:
>
>>> We could make it more descriptive of what it will do and not the reason
>>> for why it is done...
>>>
>>>
>>>    ARCH_FTRACE_SHIFT_STACK_TRACER
>> Acked-by: Will Deacon <will@kernel.org>
> Thanks Will!
>
> Here's the official patch.
>
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> Most archs (well at least x86) store the function call return address on the
> stack before storing the local variables for the function. The max stack
> tracer depends on this in its algorithm to display the stack size of each
> function it finds in the back trace.
>
> Some archs (arm64), may store the return address (from its link register)
> just before calling a nested function. There's no reason to save the link
> register on leaf functions, as it wont be updated. This breaks the algorithm
> of the max stack tracer.
>
> Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set

ARCH_FTRACE_SHIFT_STACK_TRACER is used in the code.

Jiping

> if it stores the return address (link register) after it stores the
> function's local variables, and have the stack trace shift the values of the
> mapped stack size to the appropriate functions.
>
> Link: 20190802094103.163576-1-jiping.ma2@windriver.com
>
> Reported-by: Jiping Ma <jiping.ma2@windriver.com>
> Acked-by: Will Deacon <will@kernel.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>   arch/arm64/include/asm/ftrace.h | 13 +++++++++++++
>   kernel/trace/trace_stack.c      | 14 ++++++++++++++
>   2 files changed, 27 insertions(+)
>
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 5ab5200b2bdc..d48667b04c41 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -14,6 +14,19 @@
>   #define MCOUNT_ADDR		((unsigned long)_mcount)
>   #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
>   
> +/*
> + * Currently, gcc tends to save the link register after the local variables
> + * on the stack. This causes the max stack tracer to report the function
> + * frame sizes for the wrong functions. By defining
> + * ARCH_FTRACE_SHIFT_STACK_TRACER, it will tell the stack tracer to expect
> + * to find the return address on the stack after the local variables have
> + * been set up.
> + *
> + * Note, this may change in the future, and we will need to deal with that
> + * if it were to happen.
> + */
> +#define ARCH_FTRACE_SHIFT_STACK_TRACER 1
> +
>   #ifndef __ASSEMBLY__
>   #include <linux/compat.h>
>   
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index 5d16f73898db..642a850af81a 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -158,6 +158,20 @@ static void check_stack(unsigned long ip, unsigned long *stack)
>   			i++;
>   	}
>   
> +#ifdef ARCH_FTRACE_SHIFT_STACK_TRACER
> +	/*
> +	 * Some archs will store the link register before calling
> +	 * nested functions. This means the saved return address
> +	 * comes after the local storage, and we need to shift
> +	 * for that.
> +	 */
> +	if (x > 1) {
> +		memmove(&stack_trace_index[0], &stack_trace_index[1],
> +			sizeof(stack_trace_index[0]) * (x - 1));
> +		x--;
> +	}
> +#endif
> +
>   	stack_trace_nr_entries = x;
>   
>   	if (task_stack_end_corrupted(current)) {

Re: [PATCH 1/2 v2] tracing/arm64: Have max stack tracer handle the case of return address after data

[Steven Rostedt]

On Fri, 9 Aug 2019 10:17:19 +0800
Jiping Ma <Jiping.Ma2@windriver.com> wrote:

> On 2019年08月09日 01:24, Steven Rostedt wrote:
> > On Thu, 8 Aug 2019 18:11:53 +0100
> > Will Deacon <will@kernel.org> wrote:
> >  
> >>> We could make it more descriptive of what it will do and not the reason
> >>> for why it is done...
> >>>
> >>>
> >>>    ARCH_FTRACE_SHIFT_STACK_TRACER  
> >> Acked-by: Will Deacon <will@kernel.org>  
> > Thanks Will!
> >
> > Here's the official patch.
> >
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> >
> > Most archs (well at least x86) store the function call return address on the
> > stack before storing the local variables for the function. The max stack
> > tracer depends on this in its algorithm to display the stack size of each
> > function it finds in the back trace.
> >
> > Some archs (arm64), may store the return address (from its link register)
> > just before calling a nested function. There's no reason to save the link
> > register on leaf functions, as it wont be updated. This breaks the algorithm
> > of the max stack tracer.
> >
> > Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set  
> 
> ARCH_FTRACE_SHIFT_STACK_TRACER is used in the code.

Ah, I did a s/x/y/ to the diff of the patch, but not the change log.
Thanks for pointing that out. I also need to update the comment in 2/2.

-- Steve

> 
> Jiping
> 
>

Re: [PATCH 1/2 v2] tracing/arm64: Have max stack tracer handle the case of return address after data

[Will Deacon]

Hi Steve,

On Thu, Aug 08, 2019 at 10:24:40PM -0400, Steven Rostedt wrote:
> On Fri, 9 Aug 2019 10:17:19 +0800
> Jiping Ma <Jiping.Ma2@windriver.com> wrote:
> > On 2019年08月09日 01:24, Steven Rostedt wrote:
> > > On Thu, 8 Aug 2019 18:11:53 +0100
> > > Will Deacon <will@kernel.org> wrote:
> > >  
> > >>> We could make it more descriptive of what it will do and not the reason
> > >>> for why it is done...
> > >>>
> > >>>
> > >>>    ARCH_FTRACE_SHIFT_STACK_TRACER  
> > >> Acked-by: Will Deacon <will@kernel.org>  
> > > Thanks Will!
> > >
> > > Here's the official patch.
> > >
> > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > >
> > > Most archs (well at least x86) store the function call return address on the
> > > stack before storing the local variables for the function. The max stack
> > > tracer depends on this in its algorithm to display the stack size of each
> > > function it finds in the back trace.
> > >
> > > Some archs (arm64), may store the return address (from its link register)
> > > just before calling a nested function. There's no reason to save the link
> > > register on leaf functions, as it wont be updated. This breaks the algorithm
> > > of the max stack tracer.
> > >
> > > Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set  
> > 
> > ARCH_FTRACE_SHIFT_STACK_TRACER is used in the code.
> 
> Ah, I did a s/x/y/ to the diff of the patch, but not the change log.
> Thanks for pointing that out. I also need to update the comment in 2/2.

Are you going to post another version of this or have you queued it already?
Just want to make sure it doesn't slip through the cracks.

Cheers,

Will

Re: [PATCH 1/2 v2] tracing/arm64: Have max stack tracer handle the case of return address after data

[Steven Rostedt]

On Tue, 13 Aug 2019 18:31:14 +0100
Will Deacon <will@kernel.org> wrote:

> Hi Steve,
> 
> On Thu, Aug 08, 2019 at 10:24:40PM -0400, Steven Rostedt wrote:
> > On Fri, 9 Aug 2019 10:17:19 +0800
> > Jiping Ma <Jiping.Ma2@windriver.com> wrote:  
> > > On 2019年08月09日 01:24, Steven Rostedt wrote:  
> > > > On Thu, 8 Aug 2019 18:11:53 +0100
> > > > Will Deacon <will@kernel.org> wrote:
> > > >    
> > > >>> We could make it more descriptive of what it will do and not the reason
> > > >>> for why it is done...
> > > >>>
> > > >>>
> > > >>>    ARCH_FTRACE_SHIFT_STACK_TRACER    
> > > >> Acked-by: Will Deacon <will@kernel.org>    
> > > > Thanks Will!
> > > >
> > > > Here's the official patch.
> > > >
> > > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > > >
> > > > Most archs (well at least x86) store the function call return address on the
> > > > stack before storing the local variables for the function. The max stack
> > > > tracer depends on this in its algorithm to display the stack size of each
> > > > function it finds in the back trace.
> > > >
> > > > Some archs (arm64), may store the return address (from its link register)
> > > > just before calling a nested function. There's no reason to save the link
> > > > register on leaf functions, as it wont be updated. This breaks the algorithm
> > > > of the max stack tracer.
> > > >
> > > > Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set    
> > > 
> > > ARCH_FTRACE_SHIFT_STACK_TRACER is used in the code.  
> > 
> > Ah, I did a s/x/y/ to the diff of the patch, but not the change log.
> > Thanks for pointing that out. I also need to update the comment in 2/2.  
> 
> Are you going to post another version of this or have you queued it already?
> Just want to make sure it doesn't slip through the cracks.
> 

Ah, it's in my queue. I should post a new version :-/

Thanks for the reminder.

-- Steve

Re: [PATCH 1/2 v2] tracing/arm64: Have max stack tracer handle the case of return address after data

[Mark Rutland]

On Wed, Aug 07, 2019 at 01:28:27PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Most archs (well at least x86) store the function call return address on the
> stack before storing the local variables for the function. The max stack
> tracer depends on this in its algorithm to display the stack size of each
> function it finds in the back trace.
> 
> Some archs (arm64), may store the return address (from its link register)
> just before calling a nested function. There's no reason to save the link
> register on leaf functions, as it wont be updated. This breaks the algorithm
> of the max stack tracer.
> 
> Add a new define ARCH_RET_ADDR_AFTER_LOCAL_VARS that an architecture may set
> if it stores the return address (link register) after it stores the
> function's local variables, and have the stack trace shift the values of the
> mapped stack size to the appropriate functions.
> 
> Link: 20190802094103.163576-1-jiping.ma2@windriver.com
> 
> Reported-by: Jiping Ma <jiping.ma2@windriver.com>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  arch/arm64/include/asm/ftrace.h | 13 +++++++++++++
>  kernel/trace/trace_stack.c      | 14 ++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 5ab5200b2bdc..961e98618db4 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -14,6 +14,19 @@
>  #define MCOUNT_ADDR		((unsigned long)_mcount)
>  #define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
>  
> +/*
> + * Currently, gcc tends to save the link register after the local variables
> + * on the stack. This causes the max stack tracer to report the function
> + * frame sizes for the wrong functions. By defining
> + * ARCH_RET_ADDR_AFTER_LOCAL_VARS, it will tell the stack tracer to expect
> + * to find the return address on the stack after the local variables have
> + * been set up.
> + *
> + * Note, this may change in the future, and we will need to deal with that
> + * if it were to happen.
> + */
> +#define ARCH_RET_ADDR_AFTER_LOCAL_VARS 1

FWIW (with whatever this got renamed to):

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

[PATCH 2/2 v2] tracing: Document the stack trace algorithm in the comments

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

As the max stack tracer algorithm is not that easy to understand from the
code, add comments that explain the algorithm and mentions how
ARCH_RET_ADDR_AFTER_LOCAL_VARS affects it.

Link: http://lkml.kernel.org/r/20190806123455.487ac02b@gandalf.local.home

Suggested-by: Joel Fernandes <joel@joelfernandes.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_stack.c | 98 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 40e4a88eea8f..f94a2fc567de 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -53,6 +53,104 @@ static void print_max_stack(void)
 	}
 }
 
+/*
+ * The stack tracer looks for a maximum stack at each call from a function. It
+ * registers a callback from ftrace, and in that callback it examines the stack
+ * size. It determines the stack size from the variable passed in, which is the
+ * address of a local variable in the stack_trace_call() callback function.
+ * The stack size is calculated by the address of the local variable to the top
+ * of the current stack. If that size is smaller than the currently saved max
+ * stack size, nothing more is done.
+ *
+ * If the size of the stack is greater than the maximum recorded size, then the
+ * following algorithm takes place.
+ *
+ * For architectures (like x86) that store the function's return address before
+ * saving the function's local variables, the stack will look something like
+ * this:
+ *
+ *   [ top of stack ]
+ *    0: sys call entry frame
+ *   10: return addr to entry code
+ *   11: start of sys_foo frame
+ *   20: return addr to sys_foo
+ *   21: start of kernel_func_bar frame
+ *   30: return addr to kernel_func_bar
+ *   31: [ do trace stack here ]
+ *
+ * The save_stack_trace() is called returning all the functions it finds in the
+ * current stack. Which would be (from the bottom of the stack to the top):
+ *
+ *   return addr to kernel_func_bar
+ *   return addr to sys_foo
+ *   return addr to entry code
+ *
+ * Now to figure out how much each of these functions' local variable size is,
+ * a search of the stack is made to find these values. When a match is made, it
+ * is added to the stack_dump_trace[] array. The offset into the stack is saved
+ * in the stack_trace_index[] array. The above example would show:
+ *
+ *        stack_dump_trace[]        |   stack_trace_index[]
+ *        ------------------        +   -------------------
+ *  return addr to kernel_func_bar  |          30
+ *  return addr to sys_foo          |          20
+ *  return addr to entry            |          10
+ *
+ * The print_max_stack() function above, uses these values to print the size of
+ * each function's portion of the stack.
+ *
+ *  for (i = 0; i < nr_entries; i++) {
+ *     size = i == nr_entries - 1 ? stack_trace_index[i] :
+ *                    stack_trace_index[i] - stack_trace_index[i+1]
+ *     print "%d %d %d %s\n", i, stack_trace_index[i], size, stack_dump_trace[i]);
+ *  }
+ *
+ * The above shows
+ *
+ *     depth size  location
+ *     ----- ----  --------
+ *  0    30   10   kernel_func_bar
+ *  1    20   10   sys_foo
+ *  2    10   10   entry code
+ *
+ * Now for architectures that might save the return address after the functions
+ * local variables (saving the link register before calling nested functions),
+ * this will cause the stack to look a little different:
+ *
+ * [ top of stack ]
+ *  0: sys call entry frame
+ * 10: start of sys_foo_frame
+ * 19: return addr to entry code << lr saved before calling kernel_func_bar
+ * 20: start of kernel_func_bar frame
+ * 29: return addr to sys_foo_frame << lr saved before calling next function
+ * 30: [ do trace stack here ]
+ *
+ * Although the functions returned by save_stack_trace() may be the same, the
+ * placement in the stack will be different. Using the same algorithm as above
+ * would yield:
+ *
+ *        stack_dump_trace[]        |   stack_trace_index[]
+ *        ------------------        +   -------------------
+ *  return addr to kernel_func_bar  |          30
+ *  return addr to sys_foo          |          29
+ *  return addr to entry            |          19
+ *
+ * Where the mapping is off by one:
+ *
+ *   kernel_func_bar stack frame size is 29 - 19 not 30 - 29!
+ *
+ * To fix this, if the architecture sets ARCH_RET_ADDR_AFTER_LOCAL_VARS the
+ * values in stack_trace_index[] are shifted by one to and the number of
+ * stack trace entries is decremented by one.
+ *
+ *        stack_dump_trace[]        |   stack_trace_index[]
+ *        ------------------        +   -------------------
+ *  return addr to kernel_func_bar  |          29
+ *  return addr to sys_foo          |          19
+ *
+ * Although the entry function is not displayed, the first function (sys_foo)
+ * will still include the stack size of it.
+ */
 static void check_stack(unsigned long ip, unsigned long *stack)
 {
 	unsigned long this_size, flags; unsigned long *p, *top, *start;
-- 
2.20.1

Re: [PATCH 2/2 v2] tracing: Document the stack trace algorithm in the comments

[Joel Fernandes]

On Wed, Aug 07, 2019 at 01:28:28PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> As the max stack tracer algorithm is not that easy to understand from the
> code, add comments that explain the algorithm and mentions how
> ARCH_RET_ADDR_AFTER_LOCAL_VARS affects it.
> 
> Link: http://lkml.kernel.org/r/20190806123455.487ac02b@gandalf.local.home
> 

Acked-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks!!

- Joel


> Suggested-by: Joel Fernandes <joel@joelfernandes.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  kernel/trace/trace_stack.c | 98 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index 40e4a88eea8f..f94a2fc567de 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -53,6 +53,104 @@ static void print_max_stack(void)
>  	}
>  }
>  
> +/*
> + * The stack tracer looks for a maximum stack at each call from a function. It
> + * registers a callback from ftrace, and in that callback it examines the stack
> + * size. It determines the stack size from the variable passed in, which is the
> + * address of a local variable in the stack_trace_call() callback function.
> + * The stack size is calculated by the address of the local variable to the top
> + * of the current stack. If that size is smaller than the currently saved max
> + * stack size, nothing more is done.
> + *
> + * If the size of the stack is greater than the maximum recorded size, then the
> + * following algorithm takes place.
> + *
> + * For architectures (like x86) that store the function's return address before
> + * saving the function's local variables, the stack will look something like
> + * this:
> + *
> + *   [ top of stack ]
> + *    0: sys call entry frame
> + *   10: return addr to entry code
> + *   11: start of sys_foo frame
> + *   20: return addr to sys_foo
> + *   21: start of kernel_func_bar frame
> + *   30: return addr to kernel_func_bar
> + *   31: [ do trace stack here ]
> + *
> + * The save_stack_trace() is called returning all the functions it finds in the
> + * current stack. Which would be (from the bottom of the stack to the top):
> + *
> + *   return addr to kernel_func_bar
> + *   return addr to sys_foo
> + *   return addr to entry code
> + *
> + * Now to figure out how much each of these functions' local variable size is,
> + * a search of the stack is made to find these values. When a match is made, it
> + * is added to the stack_dump_trace[] array. The offset into the stack is saved
> + * in the stack_trace_index[] array. The above example would show:
> + *
> + *        stack_dump_trace[]        |   stack_trace_index[]
> + *        ------------------        +   -------------------
> + *  return addr to kernel_func_bar  |          30
> + *  return addr to sys_foo          |          20
> + *  return addr to entry            |          10
> + *
> + * The print_max_stack() function above, uses these values to print the size of
> + * each function's portion of the stack.
> + *
> + *  for (i = 0; i < nr_entries; i++) {
> + *     size = i == nr_entries - 1 ? stack_trace_index[i] :
> + *                    stack_trace_index[i] - stack_trace_index[i+1]
> + *     print "%d %d %d %s\n", i, stack_trace_index[i], size, stack_dump_trace[i]);
> + *  }
> + *
> + * The above shows
> + *
> + *     depth size  location
> + *     ----- ----  --------
> + *  0    30   10   kernel_func_bar
> + *  1    20   10   sys_foo
> + *  2    10   10   entry code
> + *
> + * Now for architectures that might save the return address after the functions
> + * local variables (saving the link register before calling nested functions),
> + * this will cause the stack to look a little different:
> + *
> + * [ top of stack ]
> + *  0: sys call entry frame
> + * 10: start of sys_foo_frame
> + * 19: return addr to entry code << lr saved before calling kernel_func_bar
> + * 20: start of kernel_func_bar frame
> + * 29: return addr to sys_foo_frame << lr saved before calling next function
> + * 30: [ do trace stack here ]
> + *
> + * Although the functions returned by save_stack_trace() may be the same, the
> + * placement in the stack will be different. Using the same algorithm as above
> + * would yield:
> + *
> + *        stack_dump_trace[]        |   stack_trace_index[]
> + *        ------------------        +   -------------------
> + *  return addr to kernel_func_bar  |          30
> + *  return addr to sys_foo          |          29
> + *  return addr to entry            |          19
> + *
> + * Where the mapping is off by one:
> + *
> + *   kernel_func_bar stack frame size is 29 - 19 not 30 - 29!
> + *
> + * To fix this, if the architecture sets ARCH_RET_ADDR_AFTER_LOCAL_VARS the
> + * values in stack_trace_index[] are shifted by one to and the number of
> + * stack trace entries is decremented by one.
> + *
> + *        stack_dump_trace[]        |   stack_trace_index[]
> + *        ------------------        +   -------------------
> + *  return addr to kernel_func_bar  |          29
> + *  return addr to sys_foo          |          19
> + *
> + * Although the entry function is not displayed, the first function (sys_foo)
> + * will still include the stack size of it.
> + */
>  static void check_stack(unsigned long ip, unsigned long *stack)
>  {
>  	unsigned long this_size, flags; unsigned long *p, *top, *start;
> -- 
> 2.20.1
> 
>