From: "Madhavan T. Venkataraman" <[email protected]>
I have rebased this patch series on top of Mark Rutland's series on his
fork of Linux.
Remove NULL task check
======================
Currently, there is a check for a NULL task in unwind_frame(). It is not
needed since all current consumers pass a non-NULL task.
Rename unwinder functions
=========================
Rename unwinder functions to unwind*() similar to other architectures
for naming consistency.
start_backtrace() ==> unwind_init()
unwind_frame() ==> unwind_next()
walk_stackframe() ==> unwind()
Split unwind_init()
===================
Unwind initialization consists of two pieces:
1. Specialized initialization for 3 different cases - from regs,
from current task and from a blocked task. Define separate
inline init functions for these:
- unwind_init_regs()
- unwind_init_current()
- unwind_init_task()
This makes it easier to add more code to each case in the
future.
2. Common initialization for all the cases:
- unwind_init_common()
Rename struct stackframe
========================
Rename "struct stackframe" to "struct unwind_state" for better naming
and consistency.
Copy arguments to unwind_state
==============================
The arguments of arch_stack_walk() are currently passed separately
to unwind functions. Instead, copy them to struct unwind_state so
that the only argument passed to unwind functions is unwind_state.
Redefine the unwinder loop
==========================
Redefine the unwinder loop and make it simple and somewhat similar to other
architectures. Define the following:
while (unwind_continue(&state))
unwind_next(&state);
unwind_continue()
This new function implements checks to determine whether the
unwind should continue or terminate.
Reliability checks
==================
There are some kernel features and conditions that make a stack trace
unreliable. Callers may require the unwinder to detect these cases.
E.g., livepatch.
Introduce a new function called unwind_check_reliability() that will detect
these cases and set a boolean "reliable" in the stackframe. Call
unwind_check_reliability() for every frame.
Introduce the first reliability check in unwind_check_reliability() - 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.
Make unwind() return a boolean to indicate reliability of the stack trace.
SYM_CODE check
==============
This is the second reliability check implemented.
SYM_CODE functions do not follow normal calling conventions. They cannot
be unwound reliably using the frame pointer. Collect the address ranges
of these functions in a special section called "sym_code_functions".
In unwind_check_reliability(), check the return PC against these ranges. If
a match is found, then mark the stack trace 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().
arch_stack_walk_reliable()
==========================
Introduce arch_stack_walk_reliable() for ARM64. This works like
arch_stack_walk() except that it returns an error if the stack trace is
found to be unreliable.
Until all of the reliability checks are in place in
unwind_check_reliability(), arch_stack_walk_reliable() may not be used by
livepatch. But it may be used by debug and test code.
HAVE_RELIABLE_STACKTRACE
========================
Select this config for arm64. However, make it conditional on
STACK_VALIDATION. When objtool is enhanced to implement stack
validation for arm64, STACK_VALIDATION will be defined.
---
Changelog:
v12:
From Mark Brown:
- Reviewed-by for the following:
[PATCH v11 1/5] arm64: Call stack_backtrace() only from within
walk_stackframe()
[PATCH v11 2/5] arm64: Rename unwinder functions
[PATCH v11 3/5] arm64: Make the unwind loop in unwind() similar to
other architectures
[PATCH v11 5/5] arm64: Create a list of SYM_CODE functions, check
return PC against list
- Add an extra patch at the end to select HAVE_RELIABLE_STACKTRACE
just as a place holder for the review. I have added it and made
it conditional on STACK_VALIDATION which has not yet been
implemented.
- Mark had a concern about the code for the check for the final
frame being repeated in two places. I have now added a new
field called "final_fp" in struct stackframe which I compute
once in stacktrace initialization. I have added an explicit
comment that the stacktrace must terminate at the final_fp.
- Place the implementation of arch_stack_walk_reliable() in a
separate patch after all the reliability checks have been
implemented.
From Mark Rutland:
- Place the removal of the NULL task check in unwind_frame() in
a separate patch.
- Add a task field to struct stackframe so the task pointer can be
passed around via the frame instead of as a separate argument. I have
taken this a step further by copying all of the arguments to
arch_stack_walk() into struct stackframe so that only that
struct needs to be passed to unwind functions.
- Rename start_backtrace() to unwind_init() instead of unwind_start().
- Acked-by for the following:
[PATCH v11 2/5] arm64: Rename unwinder functions
- Rename "struct stackframe" to "struct unwind_state".
- Define separate inline functions for initializing the starting
FP and PC from regs, or caller, or blocked task. Don't merge
unwind_init() into unwind().
v11:
From Mark Rutland:
- Peter Zijlstra has submitted patches that make ARCH_STACKWALK
independent of STACKTRACE. Mark Rutland extracted some of the
patches from my v10 series and added his own patches and comments,
rebased it on top of Peter's changes and submitted the series.
So, I have rebased the rest of the patches from v10 on top of
Mark Rutland's changes.
- Split the renaming of the unwinder functions and annotating them
with notrace and NOKPROBE_SYMBOL(). Also, there is currently no
need to annotate unwind_start() as its caller is already annotated
properly. So, I am removing the annotation patch from the series.
This can be done separately later if deemed necessary. Similarly,
I have removed the annotations from unwind_check_reliability() and
unwind_continue().
From Nobuta Keiya:
- unwind_start() should check for final frame and not mark the
final frame unreliable.
v9, v10:
- v9 had a threading problem. So, I resent it as v10.
From me:
- Removed the word "RFC" from the subject line as I believe this
is mature enough to be a regular patch.
From Mark Brown, Mark Rutland:
- Split the patches into smaller, self-contained ones.
- Always enable STACKTRACE so that arch_stack_walk() is always
defined.
From Mark Rutland:
- Update callchain_trace() take the return value of
perf_callchain_store() into acount.
- Restore get_wchan() behavior to the original code.
- Simplify an if statement in dump_backtrace().
From Mark Brown:
- Do not abort the stack trace on the first unreliable frame.
v8:
- Synced to v5.14-rc5.
From Mark Rutland:
- Make the unwinder loop similar to other architectures.
- Keep details to within the unwinder functions and return a simple
boolean to the caller.
- Convert some of the current code that contains unwinder logic to
simply use arch_stack_walk(). I have converted all of them.
- Do not copy sym_code_functions[]. Just place it in rodata for now.
- Have the main loop check for termination conditions rather than
having unwind_frame() check for them. In other words, let
unwind_frame() assume that the fp is valid.
- Replace the big comment for SYM_CODE functions with a shorter
comment.
/*
* As SYM_CODE functions don't follow the usual calling
* conventions, we assume by default that any SYM_CODE function
* cannot be unwound reliably.
*
* Note that this includes:
*
* - Exception handlers and entry assembly
* - Trampoline assembly (e.g., ftrace, kprobes)
* - Hypervisor-related assembly
* - Hibernation-related assembly
* - CPU start-stop, suspend-resume assembly
* - Kernel relocation assembly
*/
v7:
The Mailer screwed up the threading on this. So, I have resent this
same series as version 8 with proper threading to avoid confusion.
v6:
From Mark Rutland:
- The per-frame reliability concept and flag are acceptable. But more
work is needed to make the per-frame checks more accurate and more
complete. E.g., some code reorg is being worked on that will help.
I have now removed the frame->reliable flag and deleted the whole
concept of per-frame status. This is orthogonal to this patch series.
Instead, I have improved the unwinder to return proper return codes
so a caller can take appropriate action without needing per-frame
status.
- Remove the mention of PLTs and update the comment.
I have replaced the comment above the call to __kernel_text_address()
with the comment suggested by Mark Rutland.
Other comments:
- Other comments on the per-frame stuff are not relevant because
that approach is not there anymore.
v5:
From Keiya Nobuta:
- The term blacklist(ed) is not to be used anymore. I have changed it
to unreliable. So, the function unwinder_blacklisted() has been
changed to unwinder_is_unreliable().
From Mark Brown:
- Add a comment for the "reliable" flag in struct stackframe. The
reliability attribute is not complete until all the checks are
in place. Added a comment above struct stackframe.
- Include some of the comments in the cover letter in the actual
code so that we can compare it with the reliable stack trace
requirements document for completeness. I have added a comment:
- above unwinder_is_unreliable() that lists the requirements
that are addressed by the function.
- above the __kernel_text_address() call about all the cases
the call covers.
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
================================
v11: https://lore.kernel.org/linux-arm-kernel/[email protected]/T/#t
v10: https://lore.kernel.org/linux-arm-kernel/[email protected]/T/#t
v9: Mailer screwed up the threading. Sent the same as v10 with proper threading.
v8: https://lore.kernel.org/linux-arm-kernel/[email protected]/
v7: Mailer screwed up the threading. Sent the same as v8 with proper threading.
v6: https://lore.kernel.org/linux-arm-kernel/[email protected]/
v5: https://lore.kernel.org/linux-arm-kernel/[email protected]/
v4: https://lore.kernel.org/linux-arm-kernel/[email protected]/
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 (10):
arm64: Remove NULL task check from unwind_frame()
arm64: Rename unwinder functions
arm64: Rename stackframe to unwind_state
arm64: Split unwind_init()
arm64: Copy unwind arguments to unwind_state
arm64: Make the unwind loop in unwind() similar to other architectures
arm64: Introduce stack trace reliability checks in the unwinder
arm64: Create a list of SYM_CODE functions, check return PC against
list
arm64: Introduce arch_stack_walk_reliable()
arm64: Select HAVE_RELIABLE_STACKTRACE
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/linkage.h | 12 ++
arch/arm64/include/asm/sections.h | 1 +
arch/arm64/include/asm/stacktrace.h | 23 ++-
arch/arm64/kernel/stacktrace.c | 286 +++++++++++++++++++++-------
arch/arm64/kernel/vmlinux.lds.S | 10 +
6 files changed, 266 insertions(+), 67 deletions(-)
base-commit: ec6f65fa3de02e060d9a1c7f9247a4a8ad719b58
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
Currently, there is a check for a NULL task in unwind_frame(). It is not
needed since all current consumers pass a non-NULL task.
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
---
arch/arm64/kernel/stacktrace.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 0fb58fed54cb..5f5bb35b7b41 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -69,9 +69,6 @@ static int notrace unwind_frame(struct task_struct *tsk,
unsigned long fp = frame->fp;
struct stack_info info;
- if (!tsk)
- tsk = current;
-
/* Final frame; nothing to unwind */
if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
return -ENOENT;
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
Rename unwinder functions for consistency and better naming.
- Rename start_backtrace() to unwind_init().
- Rename unwind_frame() to unwind_next().
- Rename walk_stackframe() to unwind().
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
Acked-by: Mark Rutland <[email protected]>
---
arch/arm64/kernel/stacktrace.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 5f5bb35b7b41..b980d96dccfc 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -33,8 +33,8 @@
*/
-static void start_backtrace(struct stackframe *frame, unsigned long fp,
- unsigned long pc)
+static void unwind_init(struct stackframe *frame, unsigned long fp,
+ unsigned long pc)
{
frame->fp = fp;
frame->pc = pc;
@@ -45,7 +45,7 @@ static void start_backtrace(struct stackframe *frame, unsigned long fp,
/*
* Prime the first unwind.
*
- * In unwind_frame() we'll check that the FP points to a valid stack,
+ * In unwind_next() we'll check that the FP points to a valid stack,
* which can't be STACK_TYPE_UNKNOWN, and the first unwind will be
* treated as a transition to whichever stack that happens to be. The
* prev_fp value won't be used, but we set it to 0 such that it is
@@ -63,8 +63,8 @@ static void start_backtrace(struct stackframe *frame, unsigned long fp,
* records (e.g. a cycle), determined based on the location and fp value of A
* and the location (but not the fp value) of B.
*/
-static int notrace unwind_frame(struct task_struct *tsk,
- struct stackframe *frame)
+static int notrace unwind_next(struct task_struct *tsk,
+ struct stackframe *frame)
{
unsigned long fp = frame->fp;
struct stack_info info;
@@ -104,7 +104,7 @@ static int notrace unwind_frame(struct task_struct *tsk,
/*
* Record this frame record's values and location. The prev_fp and
- * prev_type are only meaningful to the next unwind_frame() invocation.
+ * prev_type are only meaningful to the next unwind_next() invocation.
*/
frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
@@ -137,23 +137,23 @@ static int notrace unwind_frame(struct task_struct *tsk,
return 0;
}
-NOKPROBE_SYMBOL(unwind_frame);
+NOKPROBE_SYMBOL(unwind_next);
-static void notrace walk_stackframe(struct task_struct *tsk,
- struct stackframe *frame,
- bool (*fn)(void *, unsigned long), void *data)
+static void notrace unwind(struct task_struct *tsk,
+ struct stackframe *frame,
+ bool (*fn)(void *, unsigned long), void *data)
{
while (1) {
int ret;
if (!fn(data, frame->pc))
break;
- ret = unwind_frame(tsk, frame);
+ ret = unwind_next(tsk, frame);
if (ret < 0)
break;
}
}
-NOKPROBE_SYMBOL(walk_stackframe);
+NOKPROBE_SYMBOL(unwind);
static bool dump_backtrace_entry(void *arg, unsigned long where)
{
@@ -195,14 +195,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
struct stackframe frame;
if (regs)
- start_backtrace(&frame, regs->regs[29], regs->pc);
+ unwind_init(&frame, regs->regs[29], regs->pc);
else if (task == current)
- start_backtrace(&frame,
+ unwind_init(&frame,
(unsigned long)__builtin_frame_address(1),
(unsigned long)__builtin_return_address(0));
else
- start_backtrace(&frame, thread_saved_fp(task),
+ unwind_init(&frame, thread_saved_fp(task),
thread_saved_pc(task));
- walk_stackframe(task, &frame, consume_entry, cookie);
+ unwind(task, &frame, consume_entry, cookie);
}
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
Rename "struct stackframe" to "struct unwind_state" for consistency and
better naming. Accordingly, rename variable/argument "frame" to "state".
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
---
arch/arm64/include/asm/stacktrace.h | 2 +-
arch/arm64/kernel/stacktrace.c | 66 ++++++++++++++---------------
2 files changed, 34 insertions(+), 34 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 3a15d376ab36..fc828c3c5dfd 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -52,7 +52,7 @@ struct stack_info {
* associated with the most recently encountered replacement lr
* value.
*/
-struct stackframe {
+struct unwind_state {
unsigned long fp;
unsigned long pc;
DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index b980d96dccfc..a1a7ff93b84f 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -33,13 +33,13 @@
*/
-static void unwind_init(struct stackframe *frame, unsigned long fp,
+static void unwind_init(struct unwind_state *state, unsigned long fp,
unsigned long pc)
{
- frame->fp = fp;
- frame->pc = pc;
+ state->fp = fp;
+ state->pc = pc;
#ifdef CONFIG_KRETPROBES
- frame->kr_cur = NULL;
+ state->kr_cur = NULL;
#endif
/*
@@ -51,9 +51,9 @@ static void unwind_init(struct stackframe *frame, unsigned long fp,
* prev_fp value won't be used, but we set it to 0 such that it is
* definitely not an accessible stack address.
*/
- bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
- frame->prev_fp = 0;
- frame->prev_type = STACK_TYPE_UNKNOWN;
+ bitmap_zero(state->stacks_done, __NR_STACK_TYPES);
+ state->prev_fp = 0;
+ state->prev_type = STACK_TYPE_UNKNOWN;
}
/*
@@ -64,9 +64,9 @@ static void unwind_init(struct stackframe *frame, unsigned long fp,
* and the location (but not the fp value) of B.
*/
static int notrace unwind_next(struct task_struct *tsk,
- struct stackframe *frame)
+ struct unwind_state *state)
{
- unsigned long fp = frame->fp;
+ unsigned long fp = state->fp;
struct stack_info info;
/* Final frame; nothing to unwind */
@@ -79,7 +79,7 @@ static int notrace unwind_next(struct task_struct *tsk,
if (!on_accessible_stack(tsk, fp, 16, &info))
return -EINVAL;
- if (test_bit(info.type, frame->stacks_done))
+ if (test_bit(info.type, state->stacks_done))
return -EINVAL;
/*
@@ -95,27 +95,27 @@ static int notrace unwind_next(struct task_struct *tsk,
* stack to another, it's never valid to unwind back to that first
* stack.
*/
- if (info.type == frame->prev_type) {
- if (fp <= frame->prev_fp)
+ if (info.type == state->prev_type) {
+ if (fp <= state->prev_fp)
return -EINVAL;
} else {
- set_bit(frame->prev_type, frame->stacks_done);
+ set_bit(state->prev_type, state->stacks_done);
}
/*
* Record this frame record's values and location. The prev_fp and
* prev_type are only meaningful to the next unwind_next() invocation.
*/
- frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
- frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
- frame->prev_fp = fp;
- frame->prev_type = info.type;
+ state->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
+ state->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
+ state->prev_fp = fp;
+ state->prev_type = info.type;
- frame->pc = ptrauth_strip_insn_pac(frame->pc);
+ state->pc = ptrauth_strip_insn_pac(state->pc);
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
- (frame->pc == (unsigned long)return_to_handler)) {
+ (state->pc == (unsigned long)return_to_handler)) {
unsigned long orig_pc;
/*
* This is a case where function graph tracer has
@@ -123,16 +123,16 @@ static int notrace unwind_next(struct task_struct *tsk,
* to hook a function return.
* So replace it to an original value.
*/
- orig_pc = ftrace_graph_ret_addr(tsk, NULL, frame->pc,
- (void *)frame->fp);
- if (WARN_ON_ONCE(frame->pc == orig_pc))
+ orig_pc = ftrace_graph_ret_addr(tsk, NULL, state->pc,
+ (void *)state->fp);
+ if (WARN_ON_ONCE(state->pc == orig_pc))
return -EINVAL;
- frame->pc = orig_pc;
+ state->pc = orig_pc;
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
#ifdef CONFIG_KRETPROBES
- if (is_kretprobe_trampoline(frame->pc))
- frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);
+ if (is_kretprobe_trampoline(state->pc))
+ state->pc = kretprobe_find_ret_addr(tsk, (void *)state->fp, &state->kr_cur);
#endif
return 0;
@@ -140,15 +140,15 @@ static int notrace unwind_next(struct task_struct *tsk,
NOKPROBE_SYMBOL(unwind_next);
static void notrace unwind(struct task_struct *tsk,
- struct stackframe *frame,
+ struct unwind_state *state,
bool (*fn)(void *, unsigned long), void *data)
{
while (1) {
int ret;
- if (!fn(data, frame->pc))
+ if (!fn(data, state->pc))
break;
- ret = unwind_next(tsk, frame);
+ ret = unwind_next(tsk, state);
if (ret < 0)
break;
}
@@ -192,17 +192,17 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
void *cookie, struct task_struct *task,
struct pt_regs *regs)
{
- struct stackframe frame;
+ struct unwind_state state;
if (regs)
- unwind_init(&frame, regs->regs[29], regs->pc);
+ unwind_init(&state, regs->regs[29], regs->pc);
else if (task == current)
- unwind_init(&frame,
+ unwind_init(&state,
(unsigned long)__builtin_frame_address(1),
(unsigned long)__builtin_return_address(0));
else
- unwind_init(&frame, thread_saved_fp(task),
+ unwind_init(&state, thread_saved_fp(task),
thread_saved_pc(task));
- unwind(task, &frame, consume_entry, cookie);
+ unwind(task, &state, consume_entry, cookie);
}
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
unwind_init() is currently a single function that initializes all of the
unwind state. Split it into the following functions and call them
appropriately:
- unwind_init_regs() - initialize from regs passed by caller.
- unwind_init_current() - initialize for the current task from the
caller of arch_stack_walk().
- unwind_init_from_task() - initialize from the saved state of a
task other than the current task. In this case, the other
task must not be running.
- unwind_init_common() - initialize fields that are common across
the above 3 cases.
This is done so that specialized initialization can be added to each case
in the future.
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
---
arch/arm64/kernel/stacktrace.c | 50 +++++++++++++++++++++++++++-------
1 file changed, 40 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index a1a7ff93b84f..bd797e3f7789 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -33,11 +33,8 @@
*/
-static void unwind_init(struct unwind_state *state, unsigned long fp,
- unsigned long pc)
+static void unwind_init_common(struct unwind_state *state)
{
- state->fp = fp;
- state->pc = pc;
#ifdef CONFIG_KRETPROBES
state->kr_cur = NULL;
#endif
@@ -56,6 +53,40 @@ static void unwind_init(struct unwind_state *state, unsigned long fp,
state->prev_type = STACK_TYPE_UNKNOWN;
}
+/*
+ * TODO: document requirements here.
+ */
+static inline void unwind_init_regs(struct unwind_state *state,
+ struct pt_regs *regs)
+{
+ state->fp = regs->regs[29];
+ state->pc = regs->pc;
+}
+
+/*
+ * TODO: document requirements here.
+ *
+ * Note: this is always inlined, and we expect our caller to be a noinline
+ * function, such that this starts from our caller's caller.
+ */
+static __always_inline void unwind_init_current(struct unwind_state *state)
+{
+ state->fp = (unsigned long)__builtin_frame_address(1);
+ state->pc = (unsigned long)__builtin_return_address(0);
+}
+
+/*
+ * TODO: document requirements here.
+ *
+ * The caller guarantees that the task is not running.
+ */
+static inline void unwind_init_task(struct unwind_state *state,
+ struct task_struct *task)
+{
+ state->fp = thread_saved_fp(task);
+ state->pc = thread_saved_pc(task);
+}
+
/*
* Unwind from one frame record (A) to the next frame record (B).
*
@@ -194,15 +225,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
{
struct unwind_state state;
+ unwind_init_common(&state);
+
if (regs)
- unwind_init(&state, regs->regs[29], regs->pc);
+ unwind_init_regs(&state, regs);
else if (task == current)
- unwind_init(&state,
- (unsigned long)__builtin_frame_address(1),
- (unsigned long)__builtin_return_address(0));
+ unwind_init_current(&state);
else
- unwind_init(&state, thread_saved_fp(task),
- thread_saved_pc(task));
+ unwind_init_task(&state, task);
unwind(task, &state, consume_entry, cookie);
}
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
Change the loop in unwind()
===========================
Change the unwind loop in unwind() to:
while (unwind_continue(state))
unwind_next(state);
This is easy to understand and maintain.
New function unwind_continue()
==============================
Define a new function unwind_continue() that is used in the unwind loop
to check for conditions that terminate a stack trace.
The conditions checked are:
- If the bottom of the stack (final frame) has been reached,
terminate.
- If the consume_entry() function returns false, the caller of
unwind has asked to terminate the stack trace. So, terminate.
- If unwind_next() failed for some reason (like stack corruption),
terminate.
Do not return an error value from unwind_next()
===============================================
We want to check for terminating conditions only in unwind_continue() from
the unwinder loop. So, do not return an error value from unwind_next().
Simply set a flag in unwind_state and check the flag in unwind_continue().
Final FP
========
Introduce a new field "final_fp" in "struct unwind_state". Initialize this
to the final frame of the stack trace:
task_pt_regs(task)->stackframe
This is where the stacktrace must terminate if it is successful. Add an
explicit comment to that effect.
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
---
arch/arm64/include/asm/stacktrace.h | 6 +++
arch/arm64/kernel/stacktrace.c | 70 ++++++++++++++++++-----------
2 files changed, 50 insertions(+), 26 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 322817d40a75..9d1fddc26586 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -59,6 +59,10 @@ struct stack_info {
* @consume_pc Consume PC function pointer.
*
* @cookie Argument to consume_pc().
+ *
+ * @final_fp Pointer to the final frame.
+ *
+ * @failed: Unwind failed.
*/
struct unwind_state {
unsigned long fp;
@@ -73,6 +77,8 @@ struct unwind_state {
struct pt_regs *regs;
stack_trace_consume_fn consume_pc;
void *cookie;
+ unsigned long final_fp;
+ bool failed;
};
extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 3ecb8242caa5..af0949f028c9 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -60,6 +60,10 @@ static void unwind_init_common(struct unwind_state *state,
bitmap_zero(state->stacks_done, __NR_STACK_TYPES);
state->prev_fp = 0;
state->prev_type = STACK_TYPE_UNKNOWN;
+ state->failed = false;
+
+ /* Stack trace terminates here. */
+ state->final_fp = (unsigned long)task_pt_regs(task)->stackframe;
}
/*
@@ -94,6 +98,23 @@ static inline void unwind_init_task(struct unwind_state *state)
state->pc = thread_saved_pc(state->task);
}
+static bool notrace unwind_continue(struct unwind_state *state)
+{
+ if (state->failed) {
+ /* PC is suspect. Cannot consume it. */
+ return false;
+ }
+
+ if (!state->consume_pc(state->cookie, state->pc)) {
+ /* Caller terminated the unwind. */
+ state->failed = true;
+ return false;
+ }
+
+ return state->fp != state->final_fp;
+}
+NOKPROBE_SYMBOL(unwind_continue);
+
/*
* Unwind from one frame record (A) to the next frame record (B).
*
@@ -101,24 +122,26 @@ static inline void unwind_init_task(struct unwind_state *state)
* records (e.g. a cycle), determined based on the location and fp value of A
* and the location (but not the fp value) of B.
*/
-static int notrace unwind_next(struct unwind_state *state)
+static void notrace unwind_next(struct unwind_state *state)
{
struct task_struct *tsk = state->task;
unsigned long fp = state->fp;
struct stack_info info;
- /* Final frame; nothing to unwind */
- if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
- return -ENOENT;
-
- if (fp & 0x7)
- return -EINVAL;
+ if (fp & 0x7) {
+ state->failed = true;
+ return;
+ }
- if (!on_accessible_stack(tsk, fp, 16, &info))
- return -EINVAL;
+ if (!on_accessible_stack(tsk, fp, 16, &info)) {
+ state->failed = true;
+ return;
+ }
- if (test_bit(info.type, state->stacks_done))
- return -EINVAL;
+ if (test_bit(info.type, state->stacks_done)) {
+ state->failed = true;
+ return;
+ }
/*
* As stacks grow downward, any valid record on the same stack must be
@@ -134,8 +157,10 @@ static int notrace unwind_next(struct unwind_state *state)
* stack.
*/
if (info.type == state->prev_type) {
- if (fp <= state->prev_fp)
- return -EINVAL;
+ if (fp <= state->prev_fp) {
+ state->failed = true;
+ return;
+ }
} else {
set_bit(state->prev_type, state->stacks_done);
}
@@ -163,8 +188,10 @@ static int notrace unwind_next(struct unwind_state *state)
*/
orig_pc = ftrace_graph_ret_addr(tsk, NULL, state->pc,
(void *)state->fp);
- if (WARN_ON_ONCE(state->pc == orig_pc))
- return -EINVAL;
+ if (WARN_ON_ONCE(state->pc == orig_pc)) {
+ state->failed = true;
+ return;
+ }
state->pc = orig_pc;
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
@@ -172,22 +199,13 @@ static int notrace unwind_next(struct unwind_state *state)
if (is_kretprobe_trampoline(state->pc))
state->pc = kretprobe_find_ret_addr(tsk, (void *)state->fp, &state->kr_cur);
#endif
-
- return 0;
}
NOKPROBE_SYMBOL(unwind_next);
static void notrace unwind(struct unwind_state *state)
{
- while (1) {
- int ret;
-
- if (!state->consume_pc(state->cookie, state->pc))
- break;
- ret = unwind_next(state);
- if (ret < 0)
- break;
- }
+ while (unwind_continue(state))
+ unwind_next(state);
}
NOKPROBE_SYMBOL(unwind);
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
Copy the following arguments passed to arch_stack_walk() to unwind_state
so that they can be passed to unwind functions via unwind_state rather
than as separate arguments:
- task
- regs
- consume_entry
- cookie
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
---
arch/arm64/include/asm/stacktrace.h | 12 ++++++++
arch/arm64/kernel/stacktrace.c | 45 ++++++++++++++++-------------
2 files changed, 37 insertions(+), 20 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index fc828c3c5dfd..322817d40a75 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -51,6 +51,14 @@ struct stack_info {
* @kr_cur: When KRETPOLINES is selected, holds the kretprobe instance
* associated with the most recently encountered replacement lr
* value.
+ *
+ * @task: Pointer to the task structure.
+ *
+ * @regs: Registers, if any.
+ *
+ * @consume_pc Consume PC function pointer.
+ *
+ * @cookie Argument to consume_pc().
*/
struct unwind_state {
unsigned long fp;
@@ -61,6 +69,10 @@ struct unwind_state {
#ifdef CONFIG_KRETPROBES
struct llist_node *kr_cur;
#endif
+ struct task_struct *task;
+ struct pt_regs *regs;
+ stack_trace_consume_fn consume_pc;
+ void *cookie;
};
extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index bd797e3f7789..3ecb8242caa5 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -33,8 +33,17 @@
*/
-static void unwind_init_common(struct unwind_state *state)
+static void unwind_init_common(struct unwind_state *state,
+ struct task_struct *task,
+ struct pt_regs *regs,
+ stack_trace_consume_fn consume_pc,
+ void *cookie)
{
+ state->task = task;
+ state->regs = regs;
+ state->consume_pc = consume_pc;
+ state->cookie = cookie;
+
#ifdef CONFIG_KRETPROBES
state->kr_cur = NULL;
#endif
@@ -56,11 +65,10 @@ static void unwind_init_common(struct unwind_state *state)
/*
* TODO: document requirements here.
*/
-static inline void unwind_init_regs(struct unwind_state *state,
- struct pt_regs *regs)
+static inline void unwind_init_regs(struct unwind_state *state)
{
- state->fp = regs->regs[29];
- state->pc = regs->pc;
+ state->fp = state->regs->regs[29];
+ state->pc = state->regs->pc;
}
/*
@@ -80,11 +88,10 @@ static __always_inline void unwind_init_current(struct unwind_state *state)
*
* The caller guarantees that the task is not running.
*/
-static inline void unwind_init_task(struct unwind_state *state,
- struct task_struct *task)
+static inline void unwind_init_task(struct unwind_state *state)
{
- state->fp = thread_saved_fp(task);
- state->pc = thread_saved_pc(task);
+ state->fp = thread_saved_fp(state->task);
+ state->pc = thread_saved_pc(state->task);
}
/*
@@ -94,9 +101,9 @@ static inline void unwind_init_task(struct unwind_state *state,
* records (e.g. a cycle), determined based on the location and fp value of A
* and the location (but not the fp value) of B.
*/
-static int notrace unwind_next(struct task_struct *tsk,
- struct unwind_state *state)
+static int notrace unwind_next(struct unwind_state *state)
{
+ struct task_struct *tsk = state->task;
unsigned long fp = state->fp;
struct stack_info info;
@@ -170,16 +177,14 @@ static int notrace unwind_next(struct task_struct *tsk,
}
NOKPROBE_SYMBOL(unwind_next);
-static void notrace unwind(struct task_struct *tsk,
- struct unwind_state *state,
- bool (*fn)(void *, unsigned long), void *data)
+static void notrace unwind(struct unwind_state *state)
{
while (1) {
int ret;
- if (!fn(data, state->pc))
+ if (!state->consume_pc(state->cookie, state->pc))
break;
- ret = unwind_next(tsk, state);
+ ret = unwind_next(state);
if (ret < 0)
break;
}
@@ -225,14 +230,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
{
struct unwind_state state;
- unwind_init_common(&state);
+ unwind_init_common(&state, task, regs, consume_entry, cookie);
if (regs)
- unwind_init_regs(&state, regs);
+ unwind_init_regs(&state);
else if (task == current)
unwind_init_current(&state);
else
- unwind_init_task(&state, task);
+ unwind_init_task(&state);
- unwind(task, &state, consume_entry, cookie);
+ unwind(&state);
}
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
There are some kernel features and conditions that make a stack trace
unreliable. Callers may require the unwinder to detect these cases.
E.g., livepatch.
Introduce a new function called unwind_check_reliability() that will
detect these cases and set a flag in the stack frame. Call
unwind_check_reliability() for every frame in unwind().
Introduce the first reliability check in unwind_check_reliability() - 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.
Let unwind() return a boolean to indicate if the stack trace is
reliable.
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
---
arch/arm64/include/asm/stacktrace.h | 3 +++
arch/arm64/kernel/stacktrace.c | 29 +++++++++++++++++++++++++++--
2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 9d1fddc26586..47d4be69799a 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -63,6 +63,8 @@ struct stack_info {
* @final_fp Pointer to the final frame.
*
* @failed: Unwind failed.
+ *
+ * @reliable: Stack trace is reliable.
*/
struct unwind_state {
unsigned long fp;
@@ -79,6 +81,7 @@ struct unwind_state {
void *cookie;
unsigned long final_fp;
bool failed;
+ bool reliable;
};
extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index af0949f028c9..54c3396a65c3 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -18,6 +18,25 @@
#include <asm/stack_pointer.h>
#include <asm/stacktrace.h>
+/*
+ * Check the stack frame for conditions that make further unwinding unreliable.
+ */
+static void unwind_check_reliability(struct unwind_state *state)
+{
+ if (state->fp == state->final_fp) {
+ /* Final frame; no more unwind, no need to check reliability */
+ return;
+ }
+
+ /*
+ * If the PC is not a known kernel text address, then we cannot
+ * be sure that a subsequent unwind will be reliable, as we
+ * don't know that the code follows our unwind requirements.
+ */
+ if (!__kernel_text_address(state->pc))
+ state->reliable = false;
+}
+
/*
* AArch64 PCS assigns the frame pointer to x29.
*
@@ -64,6 +83,8 @@ static void unwind_init_common(struct unwind_state *state,
/* Stack trace terminates here. */
state->final_fp = (unsigned long)task_pt_regs(task)->stackframe;
+
+ state->reliable = true;
}
/*
@@ -202,10 +223,14 @@ static void notrace unwind_next(struct unwind_state *state)
}
NOKPROBE_SYMBOL(unwind_next);
-static void notrace unwind(struct unwind_state *state)
+static bool notrace unwind(struct unwind_state *state)
{
- while (unwind_continue(state))
+ unwind_check_reliability(state);
+ while (unwind_continue(state)) {
unwind_next(state);
+ unwind_check_reliability(state);
+ }
+ return !state->failed && state->reliable;
}
NOKPROBE_SYMBOL(unwind);
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
SYM_CODE functions don't follow the usual calling conventions. Check if the
return PC in a stack frame falls in any of these. If it does, consider the
stack trace unreliable.
Define a special section for unreliable 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 read-only data in
vmlinux.lds.S.
Initialization
==============
Define an early_initcall() to create a sym_code_functions[] array from
the linker data.
Unwinder check
==============
Add a reliability check in unwind_check_reliability() that compares a
return PC with sym_code_functions[]. If there is a match, then return
failure.
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
---
arch/arm64/include/asm/linkage.h | 12 +++++++
arch/arm64/include/asm/sections.h | 1 +
arch/arm64/kernel/stacktrace.c | 55 +++++++++++++++++++++++++++++++
arch/arm64/kernel/vmlinux.lds.S | 10 ++++++
4 files changed, 78 insertions(+)
diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
index 9906541a6861..616bad74e297 100644
--- a/arch/arm64/include/asm/linkage.h
+++ b/arch/arm64/include/asm/linkage.h
@@ -68,4 +68,16 @@
SYM_FUNC_END_ALIAS(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 152cb35bf9df..ac01189668c5 100644
--- a/arch/arm64/include/asm/sections.h
+++ b/arch/arm64/include/asm/sections.h
@@ -22,5 +22,6 @@ 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 __relocate_new_kernel_start[], __relocate_new_kernel_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 54c3396a65c3..1db1ccb61241 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -18,11 +18,40 @@
#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 = (unsigned long)__sym_code_functions_end -
+ (unsigned long)__sym_code_functions_start;
+
+ sym_code_functions = (struct code_range *)__sym_code_functions_start;
+ /*
+ * Order it so that sym_code_functions is not visible before
+ * num_sym_code_functions.
+ */
+ smp_mb();
+ num_sym_code_functions = size / sizeof(struct code_range);
+
+ return 0;
+}
+early_initcall(init_sym_code_functions);
+
/*
* Check the stack frame for conditions that make further unwinding unreliable.
*/
static void unwind_check_reliability(struct unwind_state *state)
{
+ const struct code_range *range;
+ unsigned long pc;
+ int i;
+
if (state->fp == state->final_fp) {
/* Final frame; no more unwind, no need to check reliability */
return;
@@ -35,6 +64,32 @@ static void unwind_check_reliability(struct unwind_state *state)
*/
if (!__kernel_text_address(state->pc))
state->reliable = false;
+
+ /*
+ * Check the return PC against sym_code_functions[]. If there is a
+ * match, then the consider the stack frame unreliable.
+ *
+ * As SYM_CODE functions don't follow the usual calling conventions,
+ * we assume by default that any SYM_CODE function cannot be unwound
+ * reliably.
+ *
+ * Note that this includes:
+ *
+ * - Exception handlers and entry assembly
+ * - Trampoline assembly (e.g., ftrace, kprobes)
+ * - Hypervisor-related assembly
+ * - Hibernation-related assembly
+ * - CPU start-stop, suspend-resume assembly
+ * - Kernel relocation assembly
+ */
+ pc = state->pc;
+ for (i = 0; i < num_sym_code_functions; i++) {
+ range = &sym_code_functions[i];
+ if (pc >= range->start && pc < range->end) {
+ state->reliable = false;
+ return;
+ }
+ }
}
/*
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 50bab186c49b..6381e75e566e 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -122,6 +122,14 @@ jiffies = jiffies_64;
#define TRAMP_TEXT
#endif
+#define SYM_CODE_FUNCTIONS \
+ . = ALIGN(16); \
+ .symcode : AT(ADDR(.symcode) - LOAD_OFFSET) { \
+ __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
@@ -209,6 +217,8 @@ SECTIONS
swapper_pg_dir = .;
. += PAGE_SIZE;
+ SYM_CODE_FUNCTIONS
+
. = ALIGN(SEGMENT_ALIGN);
__init_begin = .;
__inittext_begin = .;
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
Introduce arch_stack_walk_reliable() for ARM64. This works like
arch_stack_walk() except that it returns -EINVAL if the stack trace is not
reliable.
Until all the reliability checks are in place, arch_stack_walk_reliable()
may not be used by livepatch. But it may be used by debug and test code.
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
---
arch/arm64/kernel/stacktrace.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 1db1ccb61241..717d30833252 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -339,3 +339,27 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
unwind(&state);
}
+
+/*
+ * arch_stack_walk_reliable() may not be used for livepatch until all of
+ * the reliability checks are in place in unwind_consume(). However,
+ * debug and test code can choose to use it even if all the checks are not
+ * in place.
+ */
+noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn,
+ void *cookie,
+ struct task_struct *task)
+{
+ struct unwind_state state;
+ bool reliable;
+
+ unwind_init_common(&state, task, NULL, consume_fn, cookie);
+
+ if (task == current)
+ unwind_init_current(&state);
+ else
+ unwind_init_task(&state);
+
+ reliable = unwind(&state);
+ return reliable ? 0 : -EINVAL;
+}
--
2.25.1
From: "Madhavan T. Venkataraman" <[email protected]>
Select HAVE_RELIABLE_STACKTRACE in arm64/Kconfig to allow
arch_stack_walk_reliable() to be used.
Note that this is conditional upon STACK_VALIDATION which will be added
when frame pointer validation is implemented (say via objtool).
Signed-off-by: Madhavan T. Venkataraman <[email protected]>
---
arch/arm64/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index c4207cf9bb17..4f6312784650 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -222,6 +222,7 @@ config ARM64
select THREAD_INFO_IN_TASK
select HAVE_ARCH_USERFAULTFD_MINOR if USERFAULTFD
select TRACE_IRQFLAGS_SUPPORT
+ select HAVE_RELIABLE_STACKTRACE if FRAME_POINTER && STACK_VALIDATION
help
ARM 64-bit (AArch64) Linux support.
--
2.25.1
On Mon, Jan 03, 2022 at 10:52:05AM -0600, [email protected] wrote:
> From: "Madhavan T. Venkataraman" <[email protected]>
>
> Rename "struct stackframe" to "struct unwind_state" for consistency and
> better naming. Accordingly, rename variable/argument "frame" to "state".
Reviewed-by: Mark Brown <[email protected]>
On Mon, Jan 03, 2022 at 10:52:07AM -0600, [email protected] wrote:
> From: "Madhavan T. Venkataraman" <[email protected]>
>
> Copy the following arguments passed to arch_stack_walk() to unwind_state
> so that they can be passed to unwind functions via unwind_state rather
> than as separate arguments:
Reviwed-by: Mark Brown <[email protected]>
On Mon, Jan 03, 2022 at 10:52:09AM -0600, [email protected] wrote:
> From: "Madhavan T. Venkataraman" <[email protected]>
>
> There are some kernel features and conditions that make a stack trace
> unreliable. Callers may require the unwinder to detect these cases.
> E.g., livepatch.
Reviwed-by: Mark Brown <[email protected]>
Thanks for the review. Do you have any comments on:
[PATCH v12 04/10] arm64: Split unwind_init()
[PATCH v12 10/10] arm64: Select HAVE_RELIABLE_STACKTRACE
Madhavan
On 1/5/22 10:58 AM, Mark Brown wrote:
> On Mon, Jan 03, 2022 at 10:52:09AM -0600, [email protected] wrote:
>> From: "Madhavan T. Venkataraman" <[email protected]>
>>
>> There are some kernel features and conditions that make a stack trace
>> unreliable. Callers may require the unwinder to detect these cases.
>> E.g., livepatch.
>
> Reviwed-by: Mark Brown <[email protected]>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On Wed, Jan 05, 2022 at 05:58:59PM -0600, Madhavan T. Venkataraman wrote:
> Thanks for the review. Do you have any comments on:
>
> [PATCH v12 04/10] arm64: Split unwind_init()
> [PATCH v12 10/10] arm64: Select HAVE_RELIABLE_STACKTRACE
Not yet.
On Mon, Jan 03, 2022 at 10:52:03AM -0600, [email protected] wrote:
> From: "Madhavan T. Venkataraman" <[email protected]>
>
> Currently, there is a check for a NULL task in unwind_frame(). It is not
> needed since all current consumers pass a non-NULL task.
>
> Signed-off-by: Madhavan T. Venkataraman <[email protected]>
> Reviewed-by: Mark Brown <[email protected]>
Reviewed-by: Mark Rutland <[email protected]>
Mark.
> ---
> arch/arm64/kernel/stacktrace.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 0fb58fed54cb..5f5bb35b7b41 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -69,9 +69,6 @@ static int notrace unwind_frame(struct task_struct *tsk,
> unsigned long fp = frame->fp;
> struct stack_info info;
>
> - if (!tsk)
> - tsk = current;
> -
> /* Final frame; nothing to unwind */
> if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
> return -ENOENT;
> --
> 2.25.1
>
On Mon, Jan 03, 2022 at 10:52:04AM -0600, [email protected] wrote:
> From: "Madhavan T. Venkataraman" <[email protected]>
>
> Rename unwinder functions for consistency and better naming.
>
> - Rename start_backtrace() to unwind_init().
> - Rename unwind_frame() to unwind_next().
> - Rename walk_stackframe() to unwind().
>
> Signed-off-by: Madhavan T. Venkataraman <[email protected]>
> Reviewed-by: Mark Brown <[email protected]>
> Acked-by: Mark Rutland <[email protected]>
For consistency, to replace my prior Acked-by:
Reviewed-by: Mark Rutland <[email protected]>
Mark.
> ---
> arch/arm64/kernel/stacktrace.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 5f5bb35b7b41..b980d96dccfc 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -33,8 +33,8 @@
> */
>
>
> -static void start_backtrace(struct stackframe *frame, unsigned long fp,
> - unsigned long pc)
> +static void unwind_init(struct stackframe *frame, unsigned long fp,
> + unsigned long pc)
> {
> frame->fp = fp;
> frame->pc = pc;
> @@ -45,7 +45,7 @@ static void start_backtrace(struct stackframe *frame, unsigned long fp,
> /*
> * Prime the first unwind.
> *
> - * In unwind_frame() we'll check that the FP points to a valid stack,
> + * In unwind_next() we'll check that the FP points to a valid stack,
> * which can't be STACK_TYPE_UNKNOWN, and the first unwind will be
> * treated as a transition to whichever stack that happens to be. The
> * prev_fp value won't be used, but we set it to 0 such that it is
> @@ -63,8 +63,8 @@ static void start_backtrace(struct stackframe *frame, unsigned long fp,
> * records (e.g. a cycle), determined based on the location and fp value of A
> * and the location (but not the fp value) of B.
> */
> -static int notrace unwind_frame(struct task_struct *tsk,
> - struct stackframe *frame)
> +static int notrace unwind_next(struct task_struct *tsk,
> + struct stackframe *frame)
> {
> unsigned long fp = frame->fp;
> struct stack_info info;
> @@ -104,7 +104,7 @@ static int notrace unwind_frame(struct task_struct *tsk,
>
> /*
> * Record this frame record's values and location. The prev_fp and
> - * prev_type are only meaningful to the next unwind_frame() invocation.
> + * prev_type are only meaningful to the next unwind_next() invocation.
> */
> frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
> frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
> @@ -137,23 +137,23 @@ static int notrace unwind_frame(struct task_struct *tsk,
>
> return 0;
> }
> -NOKPROBE_SYMBOL(unwind_frame);
> +NOKPROBE_SYMBOL(unwind_next);
>
> -static void notrace walk_stackframe(struct task_struct *tsk,
> - struct stackframe *frame,
> - bool (*fn)(void *, unsigned long), void *data)
> +static void notrace unwind(struct task_struct *tsk,
> + struct stackframe *frame,
> + bool (*fn)(void *, unsigned long), void *data)
> {
> while (1) {
> int ret;
>
> if (!fn(data, frame->pc))
> break;
> - ret = unwind_frame(tsk, frame);
> + ret = unwind_next(tsk, frame);
> if (ret < 0)
> break;
> }
> }
> -NOKPROBE_SYMBOL(walk_stackframe);
> +NOKPROBE_SYMBOL(unwind);
>
> static bool dump_backtrace_entry(void *arg, unsigned long where)
> {
> @@ -195,14 +195,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
> struct stackframe frame;
>
> if (regs)
> - start_backtrace(&frame, regs->regs[29], regs->pc);
> + unwind_init(&frame, regs->regs[29], regs->pc);
> else if (task == current)
> - start_backtrace(&frame,
> + unwind_init(&frame,
> (unsigned long)__builtin_frame_address(1),
> (unsigned long)__builtin_return_address(0));
> else
> - start_backtrace(&frame, thread_saved_fp(task),
> + unwind_init(&frame, thread_saved_fp(task),
> thread_saved_pc(task));
>
> - walk_stackframe(task, &frame, consume_entry, cookie);
> + unwind(task, &frame, consume_entry, cookie);
> }
> --
> 2.25.1
>
On Mon, Jan 03, 2022 at 10:52:05AM -0600, [email protected] wrote:
> From: "Madhavan T. Venkataraman" <[email protected]>
>
> Rename "struct stackframe" to "struct unwind_state" for consistency and
> better naming. Accordingly, rename variable/argument "frame" to "state".
>
> Signed-off-by: Madhavan T. Venkataraman <[email protected]>
Thanks for this!
Reviewed-by: Mark Rutland <[email protected]>
Mark.
> ---
> arch/arm64/include/asm/stacktrace.h | 2 +-
> arch/arm64/kernel/stacktrace.c | 66 ++++++++++++++---------------
> 2 files changed, 34 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 3a15d376ab36..fc828c3c5dfd 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -52,7 +52,7 @@ struct stack_info {
> * associated with the most recently encountered replacement lr
> * value.
> */
> -struct stackframe {
> +struct unwind_state {
> unsigned long fp;
> unsigned long pc;
> DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index b980d96dccfc..a1a7ff93b84f 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -33,13 +33,13 @@
> */
>
>
> -static void unwind_init(struct stackframe *frame, unsigned long fp,
> +static void unwind_init(struct unwind_state *state, unsigned long fp,
> unsigned long pc)
> {
> - frame->fp = fp;
> - frame->pc = pc;
> + state->fp = fp;
> + state->pc = pc;
> #ifdef CONFIG_KRETPROBES
> - frame->kr_cur = NULL;
> + state->kr_cur = NULL;
> #endif
>
> /*
> @@ -51,9 +51,9 @@ static void unwind_init(struct stackframe *frame, unsigned long fp,
> * prev_fp value won't be used, but we set it to 0 such that it is
> * definitely not an accessible stack address.
> */
> - bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
> - frame->prev_fp = 0;
> - frame->prev_type = STACK_TYPE_UNKNOWN;
> + bitmap_zero(state->stacks_done, __NR_STACK_TYPES);
> + state->prev_fp = 0;
> + state->prev_type = STACK_TYPE_UNKNOWN;
> }
>
> /*
> @@ -64,9 +64,9 @@ static void unwind_init(struct stackframe *frame, unsigned long fp,
> * and the location (but not the fp value) of B.
> */
> static int notrace unwind_next(struct task_struct *tsk,
> - struct stackframe *frame)
> + struct unwind_state *state)
> {
> - unsigned long fp = frame->fp;
> + unsigned long fp = state->fp;
> struct stack_info info;
>
> /* Final frame; nothing to unwind */
> @@ -79,7 +79,7 @@ static int notrace unwind_next(struct task_struct *tsk,
> if (!on_accessible_stack(tsk, fp, 16, &info))
> return -EINVAL;
>
> - if (test_bit(info.type, frame->stacks_done))
> + if (test_bit(info.type, state->stacks_done))
> return -EINVAL;
>
> /*
> @@ -95,27 +95,27 @@ static int notrace unwind_next(struct task_struct *tsk,
> * stack to another, it's never valid to unwind back to that first
> * stack.
> */
> - if (info.type == frame->prev_type) {
> - if (fp <= frame->prev_fp)
> + if (info.type == state->prev_type) {
> + if (fp <= state->prev_fp)
> return -EINVAL;
> } else {
> - set_bit(frame->prev_type, frame->stacks_done);
> + set_bit(state->prev_type, state->stacks_done);
> }
>
> /*
> * Record this frame record's values and location. The prev_fp and
> * prev_type are only meaningful to the next unwind_next() invocation.
> */
> - frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
> - frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
> - frame->prev_fp = fp;
> - frame->prev_type = info.type;
> + state->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
> + state->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
> + state->prev_fp = fp;
> + state->prev_type = info.type;
>
> - frame->pc = ptrauth_strip_insn_pac(frame->pc);
> + state->pc = ptrauth_strip_insn_pac(state->pc);
>
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> if (tsk->ret_stack &&
> - (frame->pc == (unsigned long)return_to_handler)) {
> + (state->pc == (unsigned long)return_to_handler)) {
> unsigned long orig_pc;
> /*
> * This is a case where function graph tracer has
> @@ -123,16 +123,16 @@ static int notrace unwind_next(struct task_struct *tsk,
> * to hook a function return.
> * So replace it to an original value.
> */
> - orig_pc = ftrace_graph_ret_addr(tsk, NULL, frame->pc,
> - (void *)frame->fp);
> - if (WARN_ON_ONCE(frame->pc == orig_pc))
> + orig_pc = ftrace_graph_ret_addr(tsk, NULL, state->pc,
> + (void *)state->fp);
> + if (WARN_ON_ONCE(state->pc == orig_pc))
> return -EINVAL;
> - frame->pc = orig_pc;
> + state->pc = orig_pc;
> }
> #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> #ifdef CONFIG_KRETPROBES
> - if (is_kretprobe_trampoline(frame->pc))
> - frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);
> + if (is_kretprobe_trampoline(state->pc))
> + state->pc = kretprobe_find_ret_addr(tsk, (void *)state->fp, &state->kr_cur);
> #endif
>
> return 0;
> @@ -140,15 +140,15 @@ static int notrace unwind_next(struct task_struct *tsk,
> NOKPROBE_SYMBOL(unwind_next);
>
> static void notrace unwind(struct task_struct *tsk,
> - struct stackframe *frame,
> + struct unwind_state *state,
> bool (*fn)(void *, unsigned long), void *data)
> {
> while (1) {
> int ret;
>
> - if (!fn(data, frame->pc))
> + if (!fn(data, state->pc))
> break;
> - ret = unwind_next(tsk, frame);
> + ret = unwind_next(tsk, state);
> if (ret < 0)
> break;
> }
> @@ -192,17 +192,17 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
> void *cookie, struct task_struct *task,
> struct pt_regs *regs)
> {
> - struct stackframe frame;
> + struct unwind_state state;
>
> if (regs)
> - unwind_init(&frame, regs->regs[29], regs->pc);
> + unwind_init(&state, regs->regs[29], regs->pc);
> else if (task == current)
> - unwind_init(&frame,
> + unwind_init(&state,
> (unsigned long)__builtin_frame_address(1),
> (unsigned long)__builtin_return_address(0));
> else
> - unwind_init(&frame, thread_saved_fp(task),
> + unwind_init(&state, thread_saved_fp(task),
> thread_saved_pc(task));
>
> - unwind(task, &frame, consume_entry, cookie);
> + unwind(task, &state, consume_entry, cookie);
> }
> --
> 2.25.1
>
On Mon, Jan 03, 2022 at 10:52:06AM -0600, [email protected] wrote:
> From: "Madhavan T. Venkataraman" <[email protected]>
>
> unwind_init() is currently a single function that initializes all of the
> unwind state. Split it into the following functions and call them
> appropriately:
>
> - unwind_init_regs() - initialize from regs passed by caller.
>
> - unwind_init_current() - initialize for the current task from the
> caller of arch_stack_walk().
>
> - unwind_init_from_task() - initialize from the saved state of a
> task other than the current task. In this case, the other
> task must not be running.
>
> - unwind_init_common() - initialize fields that are common across
> the above 3 cases.
>
> This is done so that specialized initialization can be added to each case
> in the future.
>
> Signed-off-by: Madhavan T. Venkataraman <[email protected]>
> ---
> arch/arm64/kernel/stacktrace.c | 50 +++++++++++++++++++++++++++-------
> 1 file changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index a1a7ff93b84f..bd797e3f7789 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -33,11 +33,8 @@
> */
>
>
> -static void unwind_init(struct unwind_state *state, unsigned long fp,
> - unsigned long pc)
> +static void unwind_init_common(struct unwind_state *state)
> {
> - state->fp = fp;
> - state->pc = pc;
> #ifdef CONFIG_KRETPROBES
> state->kr_cur = NULL;
> #endif
> @@ -56,6 +53,40 @@ static void unwind_init(struct unwind_state *state, unsigned long fp,
> state->prev_type = STACK_TYPE_UNKNOWN;
> }
>
> +/*
> + * TODO: document requirements here.
> + */
> +static inline void unwind_init_regs(struct unwind_state *state,
> + struct pt_regs *regs)
> +{
> + state->fp = regs->regs[29];
> + state->pc = regs->pc;
> +}
When I suggested this back in:
https://lore.kernel.org/linux-arm-kernel/[email protected]/T/#md91fbfe08ceab2a02d9f5c326e17997786e53208
... my intent was that each unwind_init_from_*() helpers was the sole
initializer of the structure, and the caller only had to call one function.
That way it's not possible to construct an object with an erroneous combination
of arguments because the prototype enforces the set of arguments, and the
helper function can operate on a consistent snapshot of those arguments.
So I'd much prefer that each of these helpers called unwind_init_common(),
rather than leaving that to the caller to do. I don't mind if those pass
arguments to unwind_init_common(), or explciitly initialize their respective
fields, but I don' think the caller should have to care about unwind_init_common().
I'd also prefer the unwind_init_from*() naming I'd previously suggested, so
that it's clear which direction information is flowing.
>
> +
> +/*
> + * TODO: document requirements here.
> + *
> + * Note: this is always inlined, and we expect our caller to be a noinline
> + * function, such that this starts from our caller's caller.
> + */
> +static __always_inline void unwind_init_current(struct unwind_state *state)
> +{
> + state->fp = (unsigned long)__builtin_frame_address(1);
> + state->pc = (unsigned long)__builtin_return_address(0);
> +}
> +
> +/*
> + * TODO: document requirements here.
> + *
> + * The caller guarantees that the task is not running.
> + */
> +static inline void unwind_init_task(struct unwind_state *state,
> + struct task_struct *task)
> +{
> + state->fp = thread_saved_fp(task);
> + state->pc = thread_saved_pc(task);
> +}
> +
> /*
> * Unwind from one frame record (A) to the next frame record (B).
> *
> @@ -194,15 +225,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
> {
> struct unwind_state state;
>
> + unwind_init_common(&state);
As above, I really don't like that the caller has to call both the common
initializer and a specialized initializer here.
Thanks,
Mark.
> +
> if (regs)
> - unwind_init(&state, regs->regs[29], regs->pc);
> + unwind_init_regs(&state, regs);
> else if (task == current)
> - unwind_init(&state,
> - (unsigned long)__builtin_frame_address(1),
> - (unsigned long)__builtin_return_address(0));
> + unwind_init_current(&state);
> else
> - unwind_init(&state, thread_saved_fp(task),
> - thread_saved_pc(task));
> + unwind_init_task(&state, task);
>
> unwind(task, &state, consume_entry, cookie);
> }
> --
> 2.25.1
>
On Mon, Jan 03, 2022 at 10:52:07AM -0600, [email protected] wrote:
> From: "Madhavan T. Venkataraman" <[email protected]>
>
> Copy the following arguments passed to arch_stack_walk() to unwind_state
> so that they can be passed to unwind functions via unwind_state rather
> than as separate arguments:
>
> - task
I agree the task should be placed in the unwind state, since it's a key part of
the environment for the unwind.
> - regs
This isn't relevant in all cases, and so for now I'd strongly prefer *not* to
have this in the unwind state as it's liable to lead to confusion and get
misused.
> - consume_entry
> - cookie
These are only relevant for the invocation of the consume_entry() function, and
so similarly I do not think they should be part of the state. It's simpler for
these to be local variables.
>
> Signed-off-by: Madhavan T. Venkataraman <[email protected]>
> ---
> arch/arm64/include/asm/stacktrace.h | 12 ++++++++
> arch/arm64/kernel/stacktrace.c | 45 ++++++++++++++++-------------
> 2 files changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index fc828c3c5dfd..322817d40a75 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -51,6 +51,14 @@ struct stack_info {
> * @kr_cur: When KRETPOLINES is selected, holds the kretprobe instance
> * associated with the most recently encountered replacement lr
> * value.
> + *
> + * @task: Pointer to the task structure.
> + *
> + * @regs: Registers, if any.
> + *
> + * @consume_pc Consume PC function pointer.
> + *
> + * @cookie Argument to consume_pc().
> */
> struct unwind_state {
> unsigned long fp;
> @@ -61,6 +69,10 @@ struct unwind_state {
> #ifdef CONFIG_KRETPROBES
> struct llist_node *kr_cur;
> #endif
> + struct task_struct *task;
> + struct pt_regs *regs;
> + stack_trace_consume_fn consume_pc;
> + void *cookie;
> };
>
> extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index bd797e3f7789..3ecb8242caa5 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -33,8 +33,17 @@
> */
>
>
> -static void unwind_init_common(struct unwind_state *state)
> +static void unwind_init_common(struct unwind_state *state,
> + struct task_struct *task,
> + struct pt_regs *regs,
> + stack_trace_consume_fn consume_pc,
> + void *cookie)
> {
> + state->task = task;
> + state->regs = regs;
> + state->consume_pc = consume_pc;
> + state->cookie = cookie;
> +
> #ifdef CONFIG_KRETPROBES
> state->kr_cur = NULL;
> #endif
> @@ -56,11 +65,10 @@ static void unwind_init_common(struct unwind_state *state)
> /*
> * TODO: document requirements here.
> */
> -static inline void unwind_init_regs(struct unwind_state *state,
> - struct pt_regs *regs)
> +static inline void unwind_init_regs(struct unwind_state *state)
> {
> - state->fp = regs->regs[29];
> - state->pc = regs->pc;
> + state->fp = state->regs->regs[29];
> + state->pc = state->regs->pc;
> }
>
> /*
> @@ -80,11 +88,10 @@ static __always_inline void unwind_init_current(struct unwind_state *state)
> *
> * The caller guarantees that the task is not running.
> */
> -static inline void unwind_init_task(struct unwind_state *state,
> - struct task_struct *task)
> +static inline void unwind_init_task(struct unwind_state *state)
> {
> - state->fp = thread_saved_fp(task);
> - state->pc = thread_saved_pc(task);
> + state->fp = thread_saved_fp(state->task);
> + state->pc = thread_saved_pc(state->task);
> }
>
> /*
> @@ -94,9 +101,9 @@ static inline void unwind_init_task(struct unwind_state *state,
> * records (e.g. a cycle), determined based on the location and fp value of A
> * and the location (but not the fp value) of B.
> */
> -static int notrace unwind_next(struct task_struct *tsk,
> - struct unwind_state *state)
> +static int notrace unwind_next(struct unwind_state *state)
> {
> + struct task_struct *tsk = state->task;
> unsigned long fp = state->fp;
> struct stack_info info;
>
> @@ -170,16 +177,14 @@ static int notrace unwind_next(struct task_struct *tsk,
> }
> NOKPROBE_SYMBOL(unwind_next);
>
> -static void notrace unwind(struct task_struct *tsk,
> - struct unwind_state *state,
> - bool (*fn)(void *, unsigned long), void *data)
> +static void notrace unwind(struct unwind_state *state)
> {
> while (1) {
> int ret;
>
> - if (!fn(data, state->pc))
> + if (!state->consume_pc(state->cookie, state->pc))
> break;
> - ret = unwind_next(tsk, state);
> + ret = unwind_next(state);
> if (ret < 0)
> break;
> }
> @@ -225,14 +230,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
> {
> struct unwind_state state;
>
> - unwind_init_common(&state);
> + unwind_init_common(&state, task, regs, consume_entry, cookie);
>
> if (regs)
> - unwind_init_regs(&state, regs);
> + unwind_init_regs(&state);
> else if (task == current)
> unwind_init_current(&state);
> else
> - unwind_init_task(&state, task);
> + unwind_init_task(&state);
>
> - unwind(task, &state, consume_entry, cookie);
> + unwind(&state);
I don't like the changes here in particular since they hide the information
flow relevant to each case.
Thanks,
Mark.
> }
> --
> 2.25.1
>
On 1/6/22 10:31 AM, Mark Rutland wrote:
> On Mon, Jan 03, 2022 at 10:52:06AM -0600, [email protected] wrote:
>> From: "Madhavan T. Venkataraman" <[email protected]>
>>
>> unwind_init() is currently a single function that initializes all of the
>> unwind state. Split it into the following functions and call them
>> appropriately:
>>
>> - unwind_init_regs() - initialize from regs passed by caller.
>>
>> - unwind_init_current() - initialize for the current task from the
>> caller of arch_stack_walk().
>>
>> - unwind_init_from_task() - initialize from the saved state of a
>> task other than the current task. In this case, the other
>> task must not be running.
>>
>> - unwind_init_common() - initialize fields that are common across
>> the above 3 cases.
>>
>> This is done so that specialized initialization can be added to each case
>> in the future.
>>
>> Signed-off-by: Madhavan T. Venkataraman <[email protected]>
>> ---
>> arch/arm64/kernel/stacktrace.c | 50 +++++++++++++++++++++++++++-------
>> 1 file changed, 40 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index a1a7ff93b84f..bd797e3f7789 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -33,11 +33,8 @@
>> */
>>
>>
>> -static void unwind_init(struct unwind_state *state, unsigned long fp,
>> - unsigned long pc)
>> +static void unwind_init_common(struct unwind_state *state)
>> {
>> - state->fp = fp;
>> - state->pc = pc;
>> #ifdef CONFIG_KRETPROBES
>> state->kr_cur = NULL;
>> #endif
>> @@ -56,6 +53,40 @@ static void unwind_init(struct unwind_state *state, unsigned long fp,
>> state->prev_type = STACK_TYPE_UNKNOWN;
>> }
>>
>> +/*
>> + * TODO: document requirements here.
>> + */
>> +static inline void unwind_init_regs(struct unwind_state *state,
>> + struct pt_regs *regs)
>> +{
>> + state->fp = regs->regs[29];
>> + state->pc = regs->pc;
>> +}
>
> When I suggested this back in:
>
> https://lore.kernel.org/linux-arm-kernel/[email protected]/T/#md91fbfe08ceab2a02d9f5c326e17997786e53208
>
> ... my intent was that each unwind_init_from_*() helpers was the sole
> initializer of the structure, and the caller only had to call one function.
> That way it's not possible to construct an object with an erroneous combination
> of arguments because the prototype enforces the set of arguments, and the
> helper function can operate on a consistent snapshot of those arguments.
>
> So I'd much prefer that each of these helpers called unwind_init_common(),
> rather than leaving that to the caller to do. I don't mind if those pass
> arguments to unwind_init_common(), or explciitly initialize their respective
> fields, but I don' think the caller should have to care about unwind_init_common().
>
> I'd also prefer the unwind_init_from*() naming I'd previously suggested, so
> that it's clear which direction information is flowing.
>
OK. No problem.
>>
>> +
>> +/*
>> + * TODO: document requirements here.
>> + *
>> + * Note: this is always inlined, and we expect our caller to be a noinline
>> + * function, such that this starts from our caller's caller.
>> + */
>> +static __always_inline void unwind_init_current(struct unwind_state *state)
>> +{
>> + state->fp = (unsigned long)__builtin_frame_address(1);
>> + state->pc = (unsigned long)__builtin_return_address(0);
>> +}
>> +
>> +/*
>> + * TODO: document requirements here.
>> + *
>> + * The caller guarantees that the task is not running.
>> + */
>> +static inline void unwind_init_task(struct unwind_state *state,
>> + struct task_struct *task)
>> +{
>> + state->fp = thread_saved_fp(task);
>> + state->pc = thread_saved_pc(task);
>> +}
>> +
>> /*
>> * Unwind from one frame record (A) to the next frame record (B).
>> *
>> @@ -194,15 +225,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>> {
>> struct unwind_state state;
>>
>> + unwind_init_common(&state);
>
> As above, I really don't like that the caller has to call both the common
> initializer and a specialized initializer here.
>
OK. Will change this.
Thanks.
Madhavan
On 1/6/22 10:37 AM, Mark Rutland wrote:
> On Mon, Jan 03, 2022 at 10:52:07AM -0600, [email protected] wrote:
>> From: "Madhavan T. Venkataraman" <[email protected]>
>>
>> Copy the following arguments passed to arch_stack_walk() to unwind_state
>> so that they can be passed to unwind functions via unwind_state rather
>> than as separate arguments:
>>
>> - task
>
> I agree the task should be placed in the unwind state, since it's a key part of
> the environment for the unwind.
>
>> - regs
>
> This isn't relevant in all cases, and so for now I'd strongly prefer *not* to
> have this in the unwind state as it's liable to lead to confusion and get
> misused.
>
>> - consume_entry
>> - cookie
>
> These are only relevant for the invocation of the consume_entry() function, and
> so similarly I do not think they should be part of the state. It's simpler for
> these to be local variables.
>
OK.
>>
>> Signed-off-by: Madhavan T. Venkataraman <[email protected]>
>> ---
>> arch/arm64/include/asm/stacktrace.h | 12 ++++++++
>> arch/arm64/kernel/stacktrace.c | 45 ++++++++++++++++-------------
>> 2 files changed, 37 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
>> index fc828c3c5dfd..322817d40a75 100644
>> --- a/arch/arm64/include/asm/stacktrace.h
>> +++ b/arch/arm64/include/asm/stacktrace.h
>> @@ -51,6 +51,14 @@ struct stack_info {
>> * @kr_cur: When KRETPOLINES is selected, holds the kretprobe instance
>> * associated with the most recently encountered replacement lr
>> * value.
>> + *
>> + * @task: Pointer to the task structure.
>> + *
>> + * @regs: Registers, if any.
>> + *
>> + * @consume_pc Consume PC function pointer.
>> + *
>> + * @cookie Argument to consume_pc().
>> */
>> struct unwind_state {
>> unsigned long fp;
>> @@ -61,6 +69,10 @@ struct unwind_state {
>> #ifdef CONFIG_KRETPROBES
>> struct llist_node *kr_cur;
>> #endif
>> + struct task_struct *task;
>> + struct pt_regs *regs;
>> + stack_trace_consume_fn consume_pc;
>> + void *cookie;
>> };
>>
>> extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index bd797e3f7789..3ecb8242caa5 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -33,8 +33,17 @@
>> */
>>
>>
>> -static void unwind_init_common(struct unwind_state *state)
>> +static void unwind_init_common(struct unwind_state *state,
>> + struct task_struct *task,
>> + struct pt_regs *regs,
>> + stack_trace_consume_fn consume_pc,
>> + void *cookie)
>> {
>> + state->task = task;
>> + state->regs = regs;
>> + state->consume_pc = consume_pc;
>> + state->cookie = cookie;
>> +
>> #ifdef CONFIG_KRETPROBES
>> state->kr_cur = NULL;
>> #endif
>> @@ -56,11 +65,10 @@ static void unwind_init_common(struct unwind_state *state)
>> /*
>> * TODO: document requirements here.
>> */
>> -static inline void unwind_init_regs(struct unwind_state *state,
>> - struct pt_regs *regs)
>> +static inline void unwind_init_regs(struct unwind_state *state)
>> {
>> - state->fp = regs->regs[29];
>> - state->pc = regs->pc;
>> + state->fp = state->regs->regs[29];
>> + state->pc = state->regs->pc;
>> }
>>
>> /*
>> @@ -80,11 +88,10 @@ static __always_inline void unwind_init_current(struct unwind_state *state)
>> *
>> * The caller guarantees that the task is not running.
>> */
>> -static inline void unwind_init_task(struct unwind_state *state,
>> - struct task_struct *task)
>> +static inline void unwind_init_task(struct unwind_state *state)
>> {
>> - state->fp = thread_saved_fp(task);
>> - state->pc = thread_saved_pc(task);
>> + state->fp = thread_saved_fp(state->task);
>> + state->pc = thread_saved_pc(state->task);
>> }
>>
>> /*
>> @@ -94,9 +101,9 @@ static inline void unwind_init_task(struct unwind_state *state,
>> * records (e.g. a cycle), determined based on the location and fp value of A
>> * and the location (but not the fp value) of B.
>> */
>> -static int notrace unwind_next(struct task_struct *tsk,
>> - struct unwind_state *state)
>> +static int notrace unwind_next(struct unwind_state *state)
>> {
>> + struct task_struct *tsk = state->task;
>> unsigned long fp = state->fp;
>> struct stack_info info;
>>
>> @@ -170,16 +177,14 @@ static int notrace unwind_next(struct task_struct *tsk,
>> }
>> NOKPROBE_SYMBOL(unwind_next);
>>
>> -static void notrace unwind(struct task_struct *tsk,
>> - struct unwind_state *state,
>> - bool (*fn)(void *, unsigned long), void *data)
>> +static void notrace unwind(struct unwind_state *state)
>> {
>> while (1) {
>> int ret;
>>
>> - if (!fn(data, state->pc))
>> + if (!state->consume_pc(state->cookie, state->pc))
>> break;
>> - ret = unwind_next(tsk, state);
>> + ret = unwind_next(state);
>> if (ret < 0)
>> break;
>> }
>> @@ -225,14 +230,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>> {
>> struct unwind_state state;
>>
>> - unwind_init_common(&state);
>> + unwind_init_common(&state, task, regs, consume_entry, cookie);
>>
>> if (regs)
>> - unwind_init_regs(&state, regs);
>> + unwind_init_regs(&state);
>> else if (task == current)
>> unwind_init_current(&state);
>> else
>> - unwind_init_task(&state, task);
>> + unwind_init_task(&state);
>>
>> - unwind(task, &state, consume_entry, cookie);
>> + unwind(&state);
>
> I don't like the changes here in particular since they hide the information
> flow relevant to each case.
>
Per previous comment I agreed to, I will pass all the arguments other than task
directly.
Thanks.
Madhavan