From: "Madhavan T. Venkataraman" <[email protected]>
There are a number of places in kernel code where the stack trace is not
reliable. Enhance the unwinder to check for those cases and mark the
stack trace as unreliable. Once all of the checks are in place, the unwinder
can provide a reliable stack trace. But before this can be used for livepatch,
some other entity needs to guarantee that the frame pointers are all set up
correctly in kernel functions. objtool is currently being worked on to
address that need.
Return address check
====================
Check the return PC of every stack frame to make sure that it is a valid
kernel text address (and not some generated code, for example). If it is
not a valid kernel text address, mark the stack trace as unreliable.
Assembly functions
==================
There are a number of assembly functions in arm64. Except for a couple of
them, these functions do not have a frame pointer prolog or epilog. Also,
many of them manipulate low-level state such as registers. These functions
are, by definition, unreliable from a stack unwinding perspective. That is,
when these functions occur in a stack trace, the unwinder would not be able
to unwind through them reliably.
Assembly functions are defined as SYM_FUNC_*() functions or SYM_CODE_*()
functions. objtool peforms static analysis of SYM_FUNC functions. It ignores
SYM_CODE functions because they have low level code that is difficult to
analyze. When objtool becomes ready eventually, SYM_FUNC functions will
be analyzed and "fixed" as necessary. So, they are not "interesting" for
the reliable unwinder.
That leaves SYM_CODE functions. It is for the unwinder to deal with these
for reliable stack trace. The unwinder needs to do the following:
- Recognize SYM_CODE functions in a stack trace.
- If a particular SYM_CODE function can be unwinded through using
some special logic, then do it. E.g., the return trampoline for
Function Graph Tracing.
- Otherwise, mark the stack trace unreliable.
Previous approach
=================
Previously, I considered a per-section approach. The following sections
mostly contain SYM_CODE functions:
.entry.text
.idmap.text
.hyp.idmap.text
.hyp.text
.hibernate_exit.text
.entry.tramp.text
So, consider the whole sections unreliable. I created an array of code ranges,
one for each section. The unwinder then checks each return PC against these
sections. If there is a match, the stack trace is unreliable.
Although this approach is reasonable, there are two problems:
1. There are SYM_FUNC functions in these sections. They also
"become" unreliable in this approach. I think that we should
be able to specify which functions are reliable and which
ones are not in any section.
2. There are a few SYM_CODE functions in .text and .init.text.
These sections contain tons of functions that are reliable.
So, I considered moving the SYM_CODE functions into a separate
section called ".code.text". I am not convinced that this
approach is fine:
- We should be able to place functions in .text and
.init.text, if we wanted to.
- Moving the hypervisor related functions caused reloc
issues.
- Moving things around just to satisfy the unwinder did not
seem right.
Current approach
================
In this version, I have taken a simpler approach to solve the issues
mentioned before.
Define an ARM64 version of SYM_CODE_END() like this:
#define SYM_CODE_END(name) \
SYM_END(name, SYM_T_NONE) ;\
99: ;\
.pushsection "sym_code_functions", "aw" ;\
.quad name ;\
.quad 99b ;\
.popsection
NOTE: I use the numeric label 99 as the end address of a function.
It does not conflict with anything right now. Please let me
know if this is OK. I can add a comment that the label 99
is reserved and should not be used by assembly code.
The above macro does the usual SYM_END(). In addition, it records the
function's address range in a special section called "sym_code_functions".
This way, all SYM_CODE functions get recorded in the section automatically.
Implement an early_initcall() called init_sym_code_functions() that allocates
an array called sym_code_functions[] and copies the function ranges from the
section to the array.
Implement an unwinder_blacklisted() function that compares a return PC to the
ranges. If there is a match, return true. Else, return false.
Call unwinder_blacklisted() on every return PC from unwind_frame(). If there
is a match, then mark the stack trace as unreliable.
Last stack frame
================
If a SYM_CODE function occurs in the very last frame in the stack trace,
then the stack trace is not considered unreliable. This is because there
is no more unwinding to do. Examples:
- EL0 exception stack traces end in the top level EL0 exception
handlers.
- All kernel thread stack traces end in ret_from_fork().
Special SYM_CODE functions
==========================
The return trampolines of the Function Graph Tracer and Kretprobe can
be recognized by the unwinder. If the return PCs can be translated to the
original PCs, then, the unwinder should perform that translation before
checking for reliability. The final PC that we end up with after all the
translations is the one we need to check for reliability.
Accordingly, I have moved the reliability checks to after the logic that
handles the Function Graph Tracer.
So, the approach is - all SYM_CODE functions are unreliable. If a SYM_CODE
function is "fixed" to make it reliable, then it should become a SYM_FUNC
function. Or, if the unwinder has special logic to unwind through a SYM_CODE
function, then that can be done.
Previously, I had some extra code to handle the Function Graph Trace
return trampoline case. I have removed it. Special casing is orthogonal to
this work and can be done separately.
Special cases
=============
Some special cases need to be mentioned:
- EL1 interrupt and exception handlers end up in sym_code_ranges[].
So, all EL1 interrupt and exception stack traces will be considered
unreliable. This the correct behavior as interrupts and exceptions
can happen on any instruction including ones in the frame pointer
prolog and epilog. Unless objtool generates metadata so the unwinder
can unwind through these special cases, such stack traces will be
considered unreliable.
- A task can get preempted at the end of an interrupt. Stack traces
of preempted tasks will show the interrupt frame in the stack trace
and will be considered unreliable.
- Breakpoints are exceptions. So, all stack traces in the break point
handler (including probes) will be considered unreliable.
- All of the ftrace trampolines end up in sym_code_functions[]. All
stack traces taken from tracer functions will be considered
unreliable.
Performance
===========
Currently, unwinder_blacklisted() does a linear search through
sym_code_functions[]. If reviewers prefer, I could sort the
sym_code_functions[] array and perform a binary search for better
performance. There are about 80 entries in the array.
---
Changelog:
v4:
From Mark Brown:
- I was checking the return PC with __kernel_text_address() before
the Function Graph trace handling. Mark Brown felt that all the
reliability checks should be performed on the original return PC
once that is obtained. So, I have moved all the reliability checks
to after the Function Graph Trace handling code in the unwinder.
Basically, the unwinder should perform PC translations first (for
rhe return trampoline for Function Graph Tracing, Kretprobes, etc).
Then, the reliability checks should be applied to the resulting
PC.
- Mark said to improve the naming of the new functions so they don't
collide with existing ones. I have used a prefix "unwinder_" for
all the new functions.
From Josh Poimboeuf:
- In the error scenarios in the unwinder, the reliable flag in the
stack frame should be set. Implemented this.
- Some of the other comments are not relevant to the new code as
I have taken a different approach in the new code. That is why
I have not made those changes. E.g., Ard wanted me to add the
"const" keyword to the global section array. That array does not
exist in v4. Similarly, Mark Brown said to use ARRAY_SIZE() for
the same array in a for loop.
Other changes:
- Add a new definition for SYM_CODE_END() that adds the address
range of the function to a special section called
"sym_code_functions".
- Include the new section under initdata in vmlinux.lds.S.
- Define an early_initcall() to copy the contents of the
"sym_code_functions" section to an array by the same name.
- Define a function unwinder_blacklisted() that compares a return
PC against sym_code_sections[]. If there is a match, mark the
stack trace unreliable. Call this from unwind_frame().
v3:
- Implemented a sym_code_ranges[] array to contains sections bounds
for text sections that contain SYM_CODE_*() functions. The unwinder
checks each return PC against the sections. If it falls in any of
the sections, the stack trace is marked unreliable.
- Moved SYM_CODE functions from .text and .init.text into a new
text section called ".code.text". Added this section to
vmlinux.lds.S and sym_code_ranges[].
- Fixed the logic in the unwinder that handles Function Graph
Tracer return trampoline.
- Removed all the previous code that handles:
- ftrace entry code for traced function
- special_functions[] array that lists individual functions
- kretprobe_trampoline() special case
v2
- Removed the terminating entry { 0, 0 } in special_functions[]
and replaced it with the idiom { /* sentinel */ }.
- Change the ftrace trampoline entry ftrace_graph_call in
special_functions[] to ftrace_call + 4 and added explanatory
comments.
- Unnested #ifdefs in special_functions[] for FTRACE.
v1
- Define a bool field in struct stackframe. This will indicate if
a stack trace is reliable.
- Implement a special_functions[] array that will be populated
with special functions in which the stack trace is considered
unreliable.
- Using kallsyms_lookup(), get the address ranges for the special
functions and record them.
- Implement an is_reliable_function(pc). This function will check
if a given return PC falls in any of the special functions. If
it does, the stack trace is unreliable.
- Implement check_reliability() function that will check if a
stack frame is reliable. Call is_reliable_function() from
check_reliability().
- Before a return PC is checked against special_funtions[], it
must be validates as a proper kernel text address. Call
__kernel_text_address() from check_reliability().
- Finally, call check_reliability() from unwind_frame() for
each stack frame.
- Add EL1 exception handlers to special_functions[].
el1_sync();
el1_irq();
el1_error();
el1_sync_invalid();
el1_irq_invalid();
el1_fiq_invalid();
el1_error_invalid();
- The above functions are currently defined as LOCAL symbols.
Make them global so that they can be referenced from the
unwinder code.
- Add FTRACE trampolines to special_functions[]:
ftrace_graph_call()
ftrace_graph_caller()
return_to_handler()
- Add the kretprobe trampoline to special functions[]:
kretprobe_trampoline()
Previous versions and discussion
================================
v3: https://lore.kernel.org/linux-arm-kernel/[email protected]/
v2: https://lore.kernel.org/linux-arm-kernel/[email protected]/
v1: https://lore.kernel.org/linux-arm-kernel/[email protected]/
Madhavan T. Venkataraman (2):
arm64: Introduce stack trace reliability checks in the unwinder
arm64: Create a list of SYM_CODE functions, blacklist them in the
unwinder
arch/arm64/include/asm/linkage.h | 12 ++++
arch/arm64/include/asm/sections.h | 1 +
arch/arm64/include/asm/stacktrace.h | 4 ++
arch/arm64/kernel/stacktrace.c | 94 +++++++++++++++++++++++++++--
arch/arm64/kernel/vmlinux.lds.S | 7 +++
5 files changed, 113 insertions(+), 5 deletions(-)
base-commit: bf05bf16c76bb44ab5156223e1e58e26dfe30a88
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
The unwinder should check for the presence of various features and
conditions that can render the stack trace unreliable and mark the
the stack trace as unreliable for the benefit of the caller.
Introduce the first reliability check - If a return PC is not a valid
kernel text address, consider the stack trace unreliable. It could be
some generated code.
Other reliability checks will be added in the future.
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
---
arch/arm64/include/asm/stacktrace.h | 4 ++++
arch/arm64/kernel/stacktrace.c | 35 ++++++++++++++++++++++++-----
2 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index eb29b1fe8255..f1eab6b029f7 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -49,6 +49,8 @@ struct stack_info {
*
* @graph: When FUNCTION_GRAPH_TRACER is selected, holds the index of a
* replacement lr value in the ftrace graph stack.
+ *
+ * @reliable: Is this stack frame reliable?
*/
struct stackframe {
unsigned long fp;
@@ -59,6 +61,7 @@ struct stackframe {
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
int graph;
#endif
+ bool reliable;
};
extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
@@ -169,6 +172,7 @@ static inline void start_backtrace(struct stackframe *frame,
bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
frame->prev_fp = 0;
frame->prev_type = STACK_TYPE_UNKNOWN;
+ frame->reliable = true;
}
#endif /* __ASM_STACKTRACE_H */
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index d55bdfb7789c..d38232cab3ee 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -44,21 +44,29 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
unsigned long fp = frame->fp;
struct stack_info info;
+ frame->reliable = true;
+
/* Terminal record; nothing to unwind */
if (!fp)
return -ENOENT;
- if (fp & 0xf)
+ if (fp & 0xf) {
+ frame->reliable = false;
return -EINVAL;
+ }
if (!tsk)
tsk = current;
- if (!on_accessible_stack(tsk, fp, &info))
+ if (!on_accessible_stack(tsk, fp, &info)) {
+ frame->reliable = false;
return -EINVAL;
+ }
- if (test_bit(info.type, frame->stacks_done))
+ if (test_bit(info.type, frame->stacks_done)) {
+ frame->reliable = false;
return -EINVAL;
+ }
/*
* As stacks grow downward, any valid record on the same stack must be
@@ -74,8 +82,10 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
* stack.
*/
if (info.type == frame->prev_type) {
- if (fp <= frame->prev_fp)
+ if (fp <= frame->prev_fp) {
+ frame->reliable = false;
return -EINVAL;
+ }
} else {
set_bit(frame->prev_type, frame->stacks_done);
}
@@ -100,14 +110,29 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
* So replace it to an original value.
*/
ret_stack = ftrace_graph_get_ret_stack(tsk, frame->graph++);
- if (WARN_ON_ONCE(!ret_stack))
+ if (WARN_ON_ONCE(!ret_stack)) {
+ frame->reliable = false;
return -EINVAL;
+ }
frame->pc = ret_stack->ret;
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
frame->pc = ptrauth_strip_insn_pac(frame->pc);
+ /*
+ * Check the return PC for conditions that make unwinding unreliable.
+ * In each case, mark the stack trace as such.
+ */
+
+ /*
+ * Make sure that the return address is a proper kernel text address.
+ * A NULL or invalid return address probably means there's some
+ * generated code which __kernel_text_address() doesn't know about.
+ */
+ if (!__kernel_text_address(frame->pc))
+ frame->reliable = false;
+
return 0;
}
NOKPROBE_SYMBOL(unwind_frame);
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
The unwinder should check if the return PC falls in any function that
is considered unreliable from an unwinding perspective. If it does,
mark the stack trace unreliable.
Function types
==============
The compiler generates code for C functions and assigns the type STT_FUNC
to them.
Assembly functions are manually assigned a type:
- STT_FUNC for functions defined with SYM_FUNC*() macros
- STT_NONE for functions defined with SYM_CODE*() macros
In the future, STT_FUNC functions will be analyzed by objtool and "fixed"
as necessary. So, they are not "interesting" to the reliable unwinder in
the kernel.
That leaves SYM_CODE*() functions. These contain low-level code that is
difficult or impossible for objtool to analyze. So, objtool ignores them
leaving them to the reliable unwinder. These functions must be blacklisted
for unwinding in some way.
Blacklisting functions
======================
Define a SYM_CODE_END() macro for arm64 that adds the function address
range to a new section called "sym_code_functions".
Linker file
===========
Include the "sym_code_functions" section under initdata in vmlinux.lds.S.
Initialization
==============
Define an early_initcall() to copy the function address ranges from the
"sym_code_functions" section to an array by the same name.
Unwinder check
==============
Define a function called unwinder_blacklisted() that compares a return
PC with sym_code_functions[]. If there is a match, then mark the stack trace
as unreliable. Call unwinder_blacklisted() from unwind_frame().
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
---
arch/arm64/include/asm/linkage.h | 12 ++++++
arch/arm64/include/asm/sections.h | 1 +
arch/arm64/kernel/stacktrace.c | 61 ++++++++++++++++++++++++++++++-
arch/arm64/kernel/vmlinux.lds.S | 7 ++++
4 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
index ba89a9af820a..3b5f1fd332b0 100644
--- a/arch/arm64/include/asm/linkage.h
+++ b/arch/arm64/include/asm/linkage.h
@@ -60,4 +60,16 @@
SYM_FUNC_END(x); \
SYM_FUNC_END_ALIAS(__pi_##x)
+/*
+ * Record the address range of each SYM_CODE function in a struct code_range
+ * in a special section.
+ */
+#define SYM_CODE_END(name) \
+ SYM_END(name, SYM_T_NONE) ;\
+ 99: ;\
+ .pushsection "sym_code_functions", "aw" ;\
+ .quad name ;\
+ .quad 99b ;\
+ .popsection
+
#endif
diff --git a/arch/arm64/include/asm/sections.h b/arch/arm64/include/asm/sections.h
index 2f36b16a5b5d..29cb566f65ec 100644
--- a/arch/arm64/include/asm/sections.h
+++ b/arch/arm64/include/asm/sections.h
@@ -20,5 +20,6 @@ extern char __exittext_begin[], __exittext_end[];
extern char __irqentry_text_start[], __irqentry_text_end[];
extern char __mmuoff_data_start[], __mmuoff_data_end[];
extern char __entry_tramp_text_start[], __entry_tramp_text_end[];
+extern char __sym_code_functions_start[], __sym_code_functions_end[];
#endif /* __ASM_SECTIONS_H */
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index d38232cab3ee..f488425cacf1 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -18,6 +18,52 @@
#include <asm/stack_pointer.h>
#include <asm/stacktrace.h>
+struct code_range {
+ unsigned long start;
+ unsigned long end;
+};
+
+static struct code_range *sym_code_functions;
+static int num_sym_code_functions;
+
+int __init init_sym_code_functions(void)
+{
+ size_t size;
+
+ size = (unsigned long)__sym_code_functions_end -
+ (unsigned long)__sym_code_functions_start;
+
+ sym_code_functions = kmalloc(size, GFP_KERNEL);
+ if (!sym_code_functions)
+ return -ENOMEM;
+
+ memcpy(sym_code_functions, __sym_code_functions_start, size);
+ /* Update num_sym_code_functions after copying sym_code_functions. */
+ smp_mb();
+ num_sym_code_functions = size / sizeof(struct code_range);
+
+ return 0;
+}
+early_initcall(init_sym_code_functions);
+
+static bool unwinder_blacklisted(unsigned long pc)
+{
+ const struct code_range *range;
+ int i;
+
+ /*
+ * If sym_code_functions[] were sorted, a binary search could be
+ * done to make this more performant.
+ */
+ for (i = 0; i < num_sym_code_functions; i++) {
+ range = &sym_code_functions[i];
+ if (pc >= range->start && pc < range->end)
+ return true;
+ }
+
+ return false;
+}
+
/*
* AArch64 PCS assigns the frame pointer to x29.
*
@@ -130,7 +176,20 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
* A NULL or invalid return address probably means there's some
* generated code which __kernel_text_address() doesn't know about.
*/
- if (!__kernel_text_address(frame->pc))
+ if (!__kernel_text_address(frame->pc)) {
+ frame->reliable = false;
+ return 0;
+ }
+
+ /*
+ * If the final frame has been reached, there is no more unwinding
+ * to do. There is no need to check if the return PC is blacklisted
+ * by the unwinder.
+ */
+ if (!frame->fp)
+ return 0;
+
+ if (unwinder_blacklisted(frame->pc))
frame->reliable = false;
return 0;
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 7eea7888bb02..32e8d57397a1 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -103,6 +103,12 @@ jiffies = jiffies_64;
#define TRAMP_TEXT
#endif
+#define SYM_CODE_FUNCTIONS \
+ . = ALIGN(16); \
+ __sym_code_functions_start = .; \
+ KEEP(*(sym_code_functions)) \
+ __sym_code_functions_end = .;
+
/*
* The size of the PE/COFF section that covers the kernel image, which
* runs from _stext to _edata, must be a round multiple of the PE/COFF
@@ -218,6 +224,7 @@ SECTIONS
CON_INITCALL
INIT_RAM_FS
*(.init.altinstructions .init.bss) /* from the EFI stub */
+ SYM_CODE_FUNCTIONS
}
.exit.data : {
EXIT_DATA
--
2.25.1
Hi Madhavan,
> +static bool unwinder_blacklisted(unsigned long pc)
> +{
I've heard that the Linux community is currently avoiding the introduction of the
term 'blacklist', see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=49decddd39e5f6132ccd7d9fdc3d7c470b0061bb
Thanks & Best Regards,
Keiya Nobuta
OK. Thanks for the info. I will be more sensitive and change the name
to something more appropriate.
Madhavan
On 5/18/21 9:06 PM, [email protected] wrote:
> Hi Madhavan,
>
>> +static bool unwinder_blacklisted(unsigned long pc)
>> +{
>
> I've heard that the Linux community is currently avoiding the introduction of the
> term 'blacklist', see:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=49decddd39e5f6132ccd7d9fdc3d7c470b0061bb
>
>
> Thanks & Best Regards,
> Keiya Nobuta
>
On Sat, May 15, 2021 at 11:00:18PM -0500, [email protected] wrote:
> From: "Madhavan T. Venkataraman" <[email protected]>
>
> The unwinder should check if the return PC falls in any function that
> is considered unreliable from an unwinding perspective. If it does,
> mark the stack trace unreliable.
Other than the naming issue this makes sense to me, I'll try to go
through the first patch properly in the next few days.
Thanks a lot!
Madhavan
On 5/19/21 2:27 PM, Mark Brown wrote:
> On Sat, May 15, 2021 at 11:00:18PM -0500, [email protected] wrote:
>> From: "Madhavan T. Venkataraman" <[email protected]>
>>
>> The unwinder should check if the return PC falls in any function that
>> is considered unreliable from an unwinding perspective. If it does,
>> mark the stack trace unreliable.
>
> Other than the naming issue this makes sense to me, I'll try to go
> through the first patch properly in the next few days.
>
On Sat, May 15, 2021 at 11:00:17PM -0500, [email protected] wrote:
> Other reliability checks will be added in the future.
...
> + frame->reliable = true;
> +
All these checks are good checks but as you say there's more stuff that
we need to add (like your patch 2 here) so I'm slightly nervous about
actually setting the reliable flag here without even a comment. Equally
well there's no actual use of this until arch_stack_walk_reliable() gets
implemented so it's not like it's causing any problems and it gives us
the structure to start building up the rest of the checks.
The other thing I guess is the question of if we want to bother flagging
frames as unrelaible when we return an error; I don't see an issue with
it and it may turn out to make it easier to do something in the future
so I'm fine with that.
On 5/21/21 11:11 AM, Mark Brown wrote:
> On Sat, May 15, 2021 at 11:00:17PM -0500, [email protected] wrote:
>
>> Other reliability checks will be added in the future.
>
> ...
>
>> + frame->reliable = true;
>> +
>
> All these checks are good checks but as you say there's more stuff that
> we need to add (like your patch 2 here) so I'm slightly nervous about
> actually setting the reliable flag here without even a comment. Equally
> well there's no actual use of this until arch_stack_walk_reliable() gets
> implemented so it's not like it's causing any problems and it gives us
> the structure to start building up the rest of the checks.
>
OK. So how about changing the field from a flag to an enum that says exactly
what happened with the frame?
enum {
FRAME_NORMAL = 0,
FRAME_UNALIGNED,
FRAME_NOT_ACCESSIBLE,
FRAME_RECURSION,
FRAME_GRAPH_ERROR,
FRAME_INVALID_TEXT_ADDRESS,
FRAME_UNRELIABLE_FUNCTION,
FRAME_NUM_STATUS,
} frame_status;
struct stackframe {
...
enum frame_status status;
};
unwind_frame()
{
frame->status = FRAME_NORMAL;
Then, for each situation, change the status appropriately.
}
Eventually, arch_stack_walk_reliable() could just declare the stack trace
as unreliable if status != FRAME_NORMAL.
Also, the caller can get an exact idea of why the stack trace failed.
Is that acceptable?
> The other thing I guess is the question of if we want to bother flagging
> frames as unrelaible when we return an error; I don't see an issue with
> it and it may turn out to make it easier to do something in the future
> so I'm fine with that
Initially, I thought that there is no need to flag it for errors. But Josh
had a comment that the stack trace is indeed unreliable on errors. Again, the
word unreliable is the one causing the problem.
The above enum-based solution addresses Josh's comment as well.
Let me know if this is good.
Thanks!
Madhavan
On Sat, May 15, 2021 at 11:00:16PM -0500, [email protected] wrote:
> Special cases
> =============
>
> Some special cases need to be mentioned:
I think it'd be good if more of this cover letter, especially sections
like this which cover the tricky bits, ended up in the code somehow -
it's recorded here and will be in the list archive but that's not the
most discoverable place so increases the maintainance burden. It'd be
great to be able to compare the code directly with the reliable
stacktrace requirements document and see everything getting ticked off,
actually going all the way there might be too much and loose the code in
the comments but I think we can get closer to it than we are. Given
that a lot of this stuff rests on the denylist perhaps some comments
just before it's called would be a good place to start?
> - EL1 interrupt and exception handlers end up in sym_code_ranges[].
> So, all EL1 interrupt and exception stack traces will be considered
> unreliable. This the correct behavior as interrupts and exceptions
This stuff about exceptions and preemption is a big one, rejecting any
exceptions makes a whole host of things easier (eg, Mark Rutland raised
interactions between non-AAPCS code and PLTs as being an issue but if
we're able to reliably reject stacks featuring any kind of preemption
anyway that should sidestep the issue).
> Performance
> ===========
> Currently, unwinder_blacklisted() does a linear search through
> sym_code_functions[]. If reviewers prefer, I could sort the
> sym_code_functions[] array and perform a binary search for better
> performance. There are about 80 entries in the array.
If people are trying to live patch a very busy/big system then this
could be an issue, equally there's probably more people focused on
getting boot times as fast as possible than live patching. Deferring
the initialisation to first use would help boot times with or without
sorting, without numbers I don't actually know that sorting is worth the
effort or needs doing immediately - obvious correctness is also a
benefit! My instinct is that for now it's probably OK leaving it as a
linear scan and then revisiting if it's not adequately performant, but
I'd defer to actual users there.
On 5/21/21 12:18 PM, Mark Brown wrote:
> On Sat, May 15, 2021 at 11:00:16PM -0500, [email protected] wrote:
>
>> Special cases
>> =============
>>
>> Some special cases need to be mentioned:
>
> I think it'd be good if more of this cover letter, especially sections
> like this which cover the tricky bits, ended up in the code somehow -
> it's recorded here and will be in the list archive but that's not the
> most discoverable place so increases the maintainance burden. It'd be
> great to be able to compare the code directly with the reliable
> stacktrace requirements document and see everything getting ticked off,
> actually going all the way there might be too much and loose the code in
> the comments but I think we can get closer to it than we are. Given
> that a lot of this stuff rests on the denylist perhaps some comments
> just before it's called would be a good place to start?
>
I will add more comments in the code to make it clear.
>> - EL1 interrupt and exception handlers end up in sym_code_ranges[].
>> So, all EL1 interrupt and exception stack traces will be considered
>> unreliable. This the correct behavior as interrupts and exceptions
>
> This stuff about exceptions and preemption is a big one, rejecting any
> exceptions makes a whole host of things easier (eg, Mark Rutland raised
> interactions between non-AAPCS code and PLTs as being an issue but if
> we're able to reliably reject stacks featuring any kind of preemption
> anyway that should sidestep the issue).
>
Yes. I will include this in the code comments.
>> Performance
>> ===========
>
>> Currently, unwinder_blacklisted() does a linear search through
>> sym_code_functions[]. If reviewers prefer, I could sort the
>> sym_code_functions[] array and perform a binary search for better
>> performance. There are about 80 entries in the array.
>
> If people are trying to live patch a very busy/big system then this
> could be an issue, equally there's probably more people focused on
> getting boot times as fast as possible than live patching. Deferring
> the initialisation to first use would help boot times with or without
> sorting, without numbers I don't actually know that sorting is worth the
> effort or needs doing immediately - obvious correctness is also a
> benefit! My instinct is that for now it's probably OK leaving it as a
> linear scan and then revisiting if it's not adequately performant, but
> I'd defer to actual users there.
I have followed the example in the Kprobe deny list. I place the section
in initdata so it can be unloaded during boot. This means that I need to
copy the information before that in early_initcall().
If the initialization must be performed on first use, I probably have to
move SYM_CODE_FUNCTIONS from initdata to some other place where it will
be retained.
If you prefer this, I could do it this way.
Thanks!
Madhavan
On Fri, May 21, 2021 at 12:23:52PM -0500, Madhavan T. Venkataraman wrote:
> On 5/21/21 11:11 AM, Mark Brown wrote:
> > On Sat, May 15, 2021 at 11:00:17PM -0500, [email protected] wrote:
> >> + frame->reliable = true;
> > All these checks are good checks but as you say there's more stuff that
> > we need to add (like your patch 2 here) so I'm slightly nervous about
> OK. So how about changing the field from a flag to an enum that says exactly
> what happened with the frame?
TBH I think the code is fine, or rather will be fine when it gets as far
as actually being used - this was more a comment about when we flip this
switch.
> Also, the caller can get an exact idea of why the stack trace failed.
I'm not sure anything other than someone debugging things will care
enough to get the code out and then decode it so it seems like it'd be
more trouble than it's worth, we're unlikely to be logging the code as
standard.
> > The other thing I guess is the question of if we want to bother flagging
> > frames as unrelaible when we return an error; I don't see an issue with
> > it and it may turn out to make it easier to do something in the future
> > so I'm fine with that
> Initially, I thought that there is no need to flag it for errors. But Josh
> had a comment that the stack trace is indeed unreliable on errors. Again, the
> word unreliable is the one causing the problem.
My understanding there is that arch_stack_walk_reliable() should be
returning an error if either the unwinder detected an error or if any
frame in the stack is flagged as unreliable so from the point of view of
users it's just looking at the error code, it's more that there's no
need for arch_stack_walk_reliable() to consider the reliability
information if an error has been detected and nothing else looks at the
reliability information.
Like I say we may come up with some use for the flag in error cases in
future so I'm not opposed to keeping the accounting there.
On Fri, May 21, 2021 at 12:47:13PM -0500, Madhavan T. Venkataraman wrote:
> On 5/21/21 12:42 PM, Mark Brown wrote:
> > Like I say we may come up with some use for the flag in error cases in
> > future so I'm not opposed to keeping the accounting there.
> So, should I leave it the way it is now? Or should I not set reliable = false
> for errors? Which one do you prefer?
> Josh,
> Are you OK with not flagging reliable = false for errors in unwind_frame()?
I think it's fine to leave it as it is.
On 5/21/21 12:47 PM, Mark Brown wrote:
> On Fri, May 21, 2021 at 12:32:52PM -0500, Madhavan T. Venkataraman wrote:
>
>> I have followed the example in the Kprobe deny list. I place the section
>> in initdata so it can be unloaded during boot. This means that I need to
>> copy the information before that in early_initcall().
>
>> If the initialization must be performed on first use, I probably have to
>> move SYM_CODE_FUNCTIONS from initdata to some other place where it will
>> be retained.
>
>> If you prefer this, I could do it this way.
>
> No, I think if people are fine with this for kprobes they should be fine
> with it here too and if not we can always incrementally improve
> performance - let's just keep things simple and easy to understand for
> now.
>
OK.
Madhavan
On Fri, May 21, 2021 at 12:32:52PM -0500, Madhavan T. Venkataraman wrote:
> I have followed the example in the Kprobe deny list. I place the section
> in initdata so it can be unloaded during boot. This means that I need to
> copy the information before that in early_initcall().
> If the initialization must be performed on first use, I probably have to
> move SYM_CODE_FUNCTIONS from initdata to some other place where it will
> be retained.
> If you prefer this, I could do it this way.
No, I think if people are fine with this for kprobes they should be fine
with it here too and if not we can always incrementally improve
performance - let's just keep things simple and easy to understand for
now.
On Fri, May 21, 2021 at 01:59:16PM -0500, Madhavan T. Venkataraman wrote:
>
>
> On 5/21/21 1:48 PM, Josh Poimboeuf wrote:
> > On Fri, May 21, 2021 at 06:53:18PM +0100, Mark Brown wrote:
> >> On Fri, May 21, 2021 at 12:47:13PM -0500, Madhavan T. Venkataraman wrote:
> >>> On 5/21/21 12:42 PM, Mark Brown wrote:
> >>
> >>>> Like I say we may come up with some use for the flag in error cases in
> >>>> future so I'm not opposed to keeping the accounting there.
> >>
> >>> So, should I leave it the way it is now? Or should I not set reliable = false
> >>> for errors? Which one do you prefer?
> >>
> >>> Josh,
> >>
> >>> Are you OK with not flagging reliable = false for errors in unwind_frame()?
> >>
> >> I think it's fine to leave it as it is.
> >
> > Either way works for me, but if you remove those 'reliable = false'
> > statements for stack corruption then, IIRC, the caller would still have
> > some confusion between the end of stack error (-ENOENT) and the other
> > errors (-EINVAL).
> >
>
> I will leave it the way it is. That is, I will do reliable = false on errors
> like you suggested.
>
> > So the caller would have to know that -ENOENT really means success.
> > Which, to me, seems kind of flaky.
> >
>
> Actually, that is why -ENOENT was introduced - to indicate successful
> stack trace termination. A return value of 0 is for continuing with
> the stack trace. A non-zero value is for terminating the stack trace.
>
> So, either we return a positive value (say 1) to indicate successful
> termination. Or, we return -ENOENT to say no more stack frames left.
> I guess -ENOENT was chosen.
I see. So it's a tri-state return value, and frame->reliable is
intended to be a private interface not checked by the callers.
That makes sense, and probably fine, it's just perhaps a bit nonstandard
compared to most Linux interfaces.
--
Josh
On Fri, May 21, 2021 at 06:53:18PM +0100, Mark Brown wrote:
> On Fri, May 21, 2021 at 12:47:13PM -0500, Madhavan T. Venkataraman wrote:
> > On 5/21/21 12:42 PM, Mark Brown wrote:
>
> > > Like I say we may come up with some use for the flag in error cases in
> > > future so I'm not opposed to keeping the accounting there.
>
> > So, should I leave it the way it is now? Or should I not set reliable = false
> > for errors? Which one do you prefer?
>
> > Josh,
>
> > Are you OK with not flagging reliable = false for errors in unwind_frame()?
>
> I think it's fine to leave it as it is.
Either way works for me, but if you remove those 'reliable = false'
statements for stack corruption then, IIRC, the caller would still have
some confusion between the end of stack error (-ENOENT) and the other
errors (-EINVAL).
So the caller would have to know that -ENOENT really means success.
Which, to me, seems kind of flaky.
BTW, not sure if you've seen what we do in x86, but we have a
'frame->error' which gets set for an error, and which is cumulative
across frames. So non-fatal reliable-type errors don't necessarily have
to stop the unwind. The end result is the same as your patch, but it
seems less confusing to me because the 'error' is cumulative. But that
might be personal preference and I'd defer to the arm64 folks.
--
Josh
On Fri, May 21, 2021 at 02:11:45PM -0500, Josh Poimboeuf wrote:
> On Fri, May 21, 2021 at 01:59:16PM -0500, Madhavan T. Venkataraman wrote:
> >
> >
> > On 5/21/21 1:48 PM, Josh Poimboeuf wrote:
> > > On Fri, May 21, 2021 at 06:53:18PM +0100, Mark Brown wrote:
> > >> On Fri, May 21, 2021 at 12:47:13PM -0500, Madhavan T. Venkataraman wrote:
> > >>> On 5/21/21 12:42 PM, Mark Brown wrote:
> > >>
> > >>>> Like I say we may come up with some use for the flag in error cases in
> > >>>> future so I'm not opposed to keeping the accounting there.
> > >>
> > >>> So, should I leave it the way it is now? Or should I not set reliable = false
> > >>> for errors? Which one do you prefer?
> > >>
> > >>> Josh,
> > >>
> > >>> Are you OK with not flagging reliable = false for errors in unwind_frame()?
> > >>
> > >> I think it's fine to leave it as it is.
> > >
> > > Either way works for me, but if you remove those 'reliable = false'
> > > statements for stack corruption then, IIRC, the caller would still have
> > > some confusion between the end of stack error (-ENOENT) and the other
> > > errors (-EINVAL).
> > >
> >
> > I will leave it the way it is. That is, I will do reliable = false on errors
> > like you suggested.
> >
> > > So the caller would have to know that -ENOENT really means success.
> > > Which, to me, seems kind of flaky.
> > >
> >
> > Actually, that is why -ENOENT was introduced - to indicate successful
> > stack trace termination. A return value of 0 is for continuing with
> > the stack trace. A non-zero value is for terminating the stack trace.
> >
> > So, either we return a positive value (say 1) to indicate successful
> > termination. Or, we return -ENOENT to say no more stack frames left.
> > I guess -ENOENT was chosen.
>
> I see. So it's a tri-state return value, and frame->reliable is
> intended to be a private interface not checked by the callers.
Or is frame->reliable supposed to be checked after all? Looking at the
code again, I'm not sure.
Either way it would be good to document the interface more clearly in a
comment above the function.
--
Josh
On 5/21/21 2:16 PM, Josh Poimboeuf wrote:
> On Fri, May 21, 2021 at 02:11:45PM -0500, Josh Poimboeuf wrote:
>> On Fri, May 21, 2021 at 01:59:16PM -0500, Madhavan T. Venkataraman wrote:
>>>
>>>
>>> On 5/21/21 1:48 PM, Josh Poimboeuf wrote:
>>>> On Fri, May 21, 2021 at 06:53:18PM +0100, Mark Brown wrote:
>>>>> On Fri, May 21, 2021 at 12:47:13PM -0500, Madhavan T. Venkataraman wrote:
>>>>>> On 5/21/21 12:42 PM, Mark Brown wrote:
>>>>>
>>>>>>> Like I say we may come up with some use for the flag in error cases in
>>>>>>> future so I'm not opposed to keeping the accounting there.
>>>>>
>>>>>> So, should I leave it the way it is now? Or should I not set reliable = false
>>>>>> for errors? Which one do you prefer?
>>>>>
>>>>>> Josh,
>>>>>
>>>>>> Are you OK with not flagging reliable = false for errors in unwind_frame()?
>>>>>
>>>>> I think it's fine to leave it as it is.
>>>>
>>>> Either way works for me, but if you remove those 'reliable = false'
>>>> statements for stack corruption then, IIRC, the caller would still have
>>>> some confusion between the end of stack error (-ENOENT) and the other
>>>> errors (-EINVAL).
>>>>
>>>
>>> I will leave it the way it is. That is, I will do reliable = false on errors
>>> like you suggested.
>>>
>>>> So the caller would have to know that -ENOENT really means success.
>>>> Which, to me, seems kind of flaky.
>>>>
>>>
>>> Actually, that is why -ENOENT was introduced - to indicate successful
>>> stack trace termination. A return value of 0 is for continuing with
>>> the stack trace. A non-zero value is for terminating the stack trace.
>>>
>>> So, either we return a positive value (say 1) to indicate successful
>>> termination. Or, we return -ENOENT to say no more stack frames left.
>>> I guess -ENOENT was chosen.
>>
>> I see. So it's a tri-state return value, and frame->reliable is
>> intended to be a private interface not checked by the callers.
>
> Or is frame->reliable supposed to be checked after all? Looking at the
> code again, I'm not sure.
>
> Either way it would be good to document the interface more clearly in a
> comment above the function.
>
So, arch_stack_walk_reliable() would do this:
start_backtrace(frame);
while (...) {
if (!frame->reliable)
return error;
consume_entry(...);
ret = unwind_frame(...);
if (ret)
break;
}
if (ret == -ENOENT)
return success;
return error;
Something like that.
I will add a comment about all of this in the unwinder.
Thanks!
Madhavan
On Fri, May 21, 2021 at 02:41:56PM -0500, Madhavan T. Venkataraman wrote:
> > Or is frame->reliable supposed to be checked after all? Looking at the
> > code again, I'm not sure.
> >
> > Either way it would be good to document the interface more clearly in a
> > comment above the function.
> >
>
> So, arch_stack_walk_reliable() would do this:
>
> start_backtrace(frame);
>
> while (...) {
> if (!frame->reliable)
> return error;
>
> consume_entry(...);
>
> ret = unwind_frame(...);
>
> if (ret)
> break;
> }
>
> if (ret == -ENOENT)
> return success;
> return error;
>
>
> Something like that.
I see. So basically there are six possible combinations of return
states:
1) No error frame->reliable
2) No error !frame->reliable
3) -ENOENT frame->reliable
5) -ENOENT !frame->reliable (doesn't happen in practice)
4) Other error frame->reliable (doesn't happen in practice)
6) Other error !frame->reliable
On x86 we have fewer combinations:
1) No error state->error
2) No error !state->error
3) Error state->error
4) Error !state->error (doesn't happen in practice)
I think the x86 interface seems more robust, because it's more narrow
and has fewer edge cases. Also it doesn't have to distinguish between
error enums, which can get hairy if a downstream callee happens to
return -ENOENT for a different reason.
--
Josh
On 5/21/21 12:42 PM, Mark Brown wrote:
> On Fri, May 21, 2021 at 12:23:52PM -0500, Madhavan T. Venkataraman wrote:
>> On 5/21/21 11:11 AM, Mark Brown wrote:
>>> On Sat, May 15, 2021 at 11:00:17PM -0500, [email protected] wrote:
>
>>>> + frame->reliable = true;
>
>>> All these checks are good checks but as you say there's more stuff that
>>> we need to add (like your patch 2 here) so I'm slightly nervous about
>
>> OK. So how about changing the field from a flag to an enum that says exactly
>> what happened with the frame?
>
> TBH I think the code is fine, or rather will be fine when it gets as far
> as actually being used - this was more a comment about when we flip this
> switch.
>
OK.
>> Also, the caller can get an exact idea of why the stack trace failed.
>
> I'm not sure anything other than someone debugging things will care
> enough to get the code out and then decode it so it seems like it'd be
> more trouble than it's worth, we're unlikely to be logging the code as
> standard.
>
OK.
>>> The other thing I guess is the question of if we want to bother flagging
>>> frames as unrelaible when we return an error; I don't see an issue with
>>> it and it may turn out to make it easier to do something in the future
>>> so I'm fine with that
>
>> Initially, I thought that there is no need to flag it for errors. But Josh
>> had a comment that the stack trace is indeed unreliable on errors. Again, the
>> word unreliable is the one causing the problem.
>
> My understanding there is that arch_stack_walk_reliable() should be
> returning an error if either the unwinder detected an error or if any
> frame in the stack is flagged as unreliable so from the point of view of
> users it's just looking at the error code, it's more that there's no
> need for arch_stack_walk_reliable() to consider the reliability
> information if an error has been detected and nothing else looks at the
> reliability information.
>
> Like I say we may come up with some use for the flag in error cases in
> future so I'm not opposed to keeping the accounting there.
>
So, should I leave it the way it is now? Or should I not set reliable = false
for errors? Which one do you prefer?
Josh,
Are you OK with not flagging reliable = false for errors in unwind_frame()?
Madhavan
On 5/21/21 1:48 PM, Josh Poimboeuf wrote:
> On Fri, May 21, 2021 at 06:53:18PM +0100, Mark Brown wrote:
>> On Fri, May 21, 2021 at 12:47:13PM -0500, Madhavan T. Venkataraman wrote:
>>> On 5/21/21 12:42 PM, Mark Brown wrote:
>>
>>>> Like I say we may come up with some use for the flag in error cases in
>>>> future so I'm not opposed to keeping the accounting there.
>>
>>> So, should I leave it the way it is now? Or should I not set reliable = false
>>> for errors? Which one do you prefer?
>>
>>> Josh,
>>
>>> Are you OK with not flagging reliable = false for errors in unwind_frame()?
>>
>> I think it's fine to leave it as it is.
>
> Either way works for me, but if you remove those 'reliable = false'
> statements for stack corruption then, IIRC, the caller would still have
> some confusion between the end of stack error (-ENOENT) and the other
> errors (-EINVAL).
>
I will leave it the way it is. That is, I will do reliable = false on errors
like you suggested.
> So the caller would have to know that -ENOENT really means success.
> Which, to me, seems kind of flaky.
>
Actually, that is why -ENOENT was introduced - to indicate successful
stack trace termination. A return value of 0 is for continuing with
the stack trace. A non-zero value is for terminating the stack trace.
So, either we return a positive value (say 1) to indicate successful
termination. Or, we return -ENOENT to say no more stack frames left.
I guess -ENOENT was chosen.
> BTW, not sure if you've seen what we do in x86, but we have a
> 'frame->error' which gets set for an error, and which is cumulative
> across frames. So non-fatal reliable-type errors don't necessarily have
> to stop the unwind. The end result is the same as your patch, but it
> seems less confusing to me because the 'error' is cumulative. But that
> might be personal preference and I'd defer to the arm64 folks.
>
OK. I will wait to see if any arm64 folks have an opinion on this.
I am fine with any approach.
Madhavan
On 5/21/21 12:53 PM, Mark Brown wrote:
> On Fri, May 21, 2021 at 12:47:13PM -0500, Madhavan T. Venkataraman wrote:
>> On 5/21/21 12:42 PM, Mark Brown wrote:
>
>>> Like I say we may come up with some use for the flag in error cases in
>>> future so I'm not opposed to keeping the accounting there.
>
>> So, should I leave it the way it is now? Or should I not set reliable = false
>> for errors? Which one do you prefer?
>
>> Josh,
>
>> Are you OK with not flagging reliable = false for errors in unwind_frame()?
>
> I think it's fine to leave it as it is.
>
OK. I will address the comments so far and send out v5.
Thanks.
Madhavan