* [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe()
2021-11-23 19:37 ` [PATCH v11 0/5] arm64: Reorganize the unwinder and implement stack trace reliability checks madvenka
@ 2021-11-23 19:37 ` madvenka
2021-11-25 13:48 ` Mark Brown
2021-11-30 15:05 ` Mark Rutland
2021-11-23 19:37 ` [PATCH v11 2/5] arm64: Rename unwinder functions madvenka
` (3 subsequent siblings)
4 siblings, 2 replies; 21+ messages in thread
From: madvenka @ 2021-11-23 19:37 UTC (permalink / raw)
To: mark.rutland, broonie, jpoimboe, ardb, nobuta.keiya,
sjitindarsingh, catalin.marinas, will, jmorris, linux-arm-kernel,
live-patching, linux-kernel, madvenka
From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
Currently, arch_stack_walk() calls start_backtrace() and walk_stackframe()
separately. There is no need to do that. Instead, call start_backtrace()
from within walk_stackframe(). In other words, walk_stackframe() is the only
unwind function a consumer needs to call.
Currently, the only consumer is arch_stack_walk(). In the future,
arch_stack_walk_reliable() will be another consumer.
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.
Use struct stackframe only within the unwind functions.
Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
---
arch/arm64/kernel/stacktrace.c | 41 ++++++++++++++++++----------------
1 file changed, 22 insertions(+), 19 deletions(-)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 0fb58fed54cb..7217c4f63ef7 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;
@@ -143,15 +140,19 @@ static int notrace unwind_frame(struct task_struct *tsk,
NOKPROBE_SYMBOL(unwind_frame);
static void notrace walk_stackframe(struct task_struct *tsk,
- struct stackframe *frame,
+ unsigned long fp, unsigned long pc,
bool (*fn)(void *, unsigned long), void *data)
{
+ struct stackframe frame;
+
+ start_backtrace(&frame, fp, pc);
+
while (1) {
int ret;
- if (!fn(data, frame->pc))
+ if (!fn(data, frame.pc))
break;
- ret = unwind_frame(tsk, frame);
+ ret = unwind_frame(tsk, &frame);
if (ret < 0)
break;
}
@@ -195,17 +196,19 @@ 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;
-
- if (regs)
- start_backtrace(&frame, regs->regs[29], regs->pc);
- else if (task == current)
- start_backtrace(&frame,
- (unsigned long)__builtin_frame_address(1),
- (unsigned long)__builtin_return_address(0));
- else
- start_backtrace(&frame, thread_saved_fp(task),
- thread_saved_pc(task));
-
- walk_stackframe(task, &frame, consume_entry, cookie);
+ unsigned long fp, pc;
+
+ if (regs) {
+ fp = regs->regs[29];
+ pc = regs->pc;
+ } else if (task == current) {
+ /* Skip arch_stack_walk() in the stack trace. */
+ fp = (unsigned long)__builtin_frame_address(1);
+ pc = (unsigned long)__builtin_return_address(0);
+ } else {
+ /* Caller guarantees that the task is not running. */
+ fp = thread_saved_fp(task);
+ pc = thread_saved_pc(task);
+ }
+ walk_stackframe(task, fp, pc, consume_entry, cookie);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe()
2021-11-23 19:37 ` [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe() madvenka
@ 2021-11-25 13:48 ` Mark Brown
2021-11-30 15:05 ` Mark Rutland
1 sibling, 0 replies; 21+ messages in thread
From: Mark Brown @ 2021-11-25 13:48 UTC (permalink / raw)
To: madvenka
Cc: mark.rutland, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 477 bytes --]
On Tue, Nov 23, 2021 at 01:37:19PM -0600, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>
> Currently, arch_stack_walk() calls start_backtrace() and walk_stackframe()
> separately. There is no need to do that. Instead, call start_backtrace()
> from within walk_stackframe(). In other words, walk_stackframe() is the only
> unwind function a consumer needs to call.
Reviewed-by: Mark Brown <broonie@kernel.org>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe()
2021-11-23 19:37 ` [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe() madvenka
2021-11-25 13:48 ` Mark Brown
@ 2021-11-30 15:05 ` Mark Rutland
2021-11-30 17:13 ` Madhavan T. Venkataraman
1 sibling, 1 reply; 21+ messages in thread
From: Mark Rutland @ 2021-11-30 15:05 UTC (permalink / raw)
To: madvenka
Cc: broonie, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
On Tue, Nov 23, 2021 at 01:37:19PM -0600, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>
> Currently, arch_stack_walk() calls start_backtrace() and walk_stackframe()
> separately. There is no need to do that. Instead, call start_backtrace()
> from within walk_stackframe(). In other words, walk_stackframe() is the only
> unwind function a consumer needs to call.
>
> Currently, the only consumer is arch_stack_walk(). In the future,
> arch_stack_walk_reliable() will be another consumer.
>
> 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.
Can you split the NULL check change into a preparatory patch? That change is
fine in isolation (and easier to review/ack), and it's nicer for future
bisection to not group that with unrelated changes.
> Use struct stackframe only within the unwind functions.
>
> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> ---
> arch/arm64/kernel/stacktrace.c | 41 ++++++++++++++++++----------------
> 1 file changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 0fb58fed54cb..7217c4f63ef7 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;
> @@ -143,15 +140,19 @@ static int notrace unwind_frame(struct task_struct *tsk,
> NOKPROBE_SYMBOL(unwind_frame);
>
> static void notrace walk_stackframe(struct task_struct *tsk,
> - struct stackframe *frame,
> + unsigned long fp, unsigned long pc,
> bool (*fn)(void *, unsigned long), void *data)
> {
> + struct stackframe frame;
> +
> + start_backtrace(&frame, fp, pc);
> +
> while (1) {
> int ret;
>
> - if (!fn(data, frame->pc))
> + if (!fn(data, frame.pc))
> break;
> - ret = unwind_frame(tsk, frame);
> + ret = unwind_frame(tsk, &frame);
> if (ret < 0)
> break;
> }
> @@ -195,17 +196,19 @@ 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;
> -
> - if (regs)
> - start_backtrace(&frame, regs->regs[29], regs->pc);
> - else if (task == current)
> - start_backtrace(&frame,
> - (unsigned long)__builtin_frame_address(1),
> - (unsigned long)__builtin_return_address(0));
> - else
> - start_backtrace(&frame, thread_saved_fp(task),
> - thread_saved_pc(task));
> -
> - walk_stackframe(task, &frame, consume_entry, cookie);
> + unsigned long fp, pc;
> +
> + if (regs) {
> + fp = regs->regs[29];
> + pc = regs->pc;
> + } else if (task == current) {
> + /* Skip arch_stack_walk() in the stack trace. */
> + fp = (unsigned long)__builtin_frame_address(1);
> + pc = (unsigned long)__builtin_return_address(0);
> + } else {
> + /* Caller guarantees that the task is not running. */
> + fp = thread_saved_fp(task);
> + pc = thread_saved_pc(task);
> + }
> + walk_stackframe(task, fp, pc, consume_entry, cookie);
I'd prefer to leave this as-is. The new and old structure are largely
equivalent, so we haven't made this any simpler, but we have added more
arguments to walk_stackframe().
One thing I *would* like to do is move tsk into strcut stackframe, so we only
need to pass that around, which'll make it easier to refactor the core unwind
logic.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe()
2021-11-30 15:05 ` Mark Rutland
@ 2021-11-30 17:13 ` Madhavan T. Venkataraman
2021-11-30 18:29 ` Mark Rutland
0 siblings, 1 reply; 21+ messages in thread
From: Madhavan T. Venkataraman @ 2021-11-30 17:13 UTC (permalink / raw)
To: Mark Rutland
Cc: broonie, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
On 11/30/21 9:05 AM, Mark Rutland wrote:
> On Tue, Nov 23, 2021 at 01:37:19PM -0600, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> Currently, arch_stack_walk() calls start_backtrace() and walk_stackframe()
>> separately. There is no need to do that. Instead, call start_backtrace()
>> from within walk_stackframe(). In other words, walk_stackframe() is the only
>> unwind function a consumer needs to call.
>>
>> Currently, the only consumer is arch_stack_walk(). In the future,
>> arch_stack_walk_reliable() will be another consumer.
>>
>> 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.
>
> Can you split the NULL check change into a preparatory patch? That change is
> fine in isolation (and easier to review/ack), and it's nicer for future
> bisection to not group that with unrelated changes.
>
Will do this in the next version.
>> Use struct stackframe only within the unwind functions.
>>
>> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
>> ---
>> arch/arm64/kernel/stacktrace.c | 41 ++++++++++++++++++----------------
>> 1 file changed, 22 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index 0fb58fed54cb..7217c4f63ef7 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;
>> @@ -143,15 +140,19 @@ static int notrace unwind_frame(struct task_struct *tsk,
>> NOKPROBE_SYMBOL(unwind_frame);
>>
>> static void notrace walk_stackframe(struct task_struct *tsk,
>> - struct stackframe *frame,
>> + unsigned long fp, unsigned long pc,
>> bool (*fn)(void *, unsigned long), void *data)
>> {
>> + struct stackframe frame;
>> +
>> + start_backtrace(&frame, fp, pc);
>> +
>> while (1) {
>> int ret;
>>
>> - if (!fn(data, frame->pc))
>> + if (!fn(data, frame.pc))
>> break;
>> - ret = unwind_frame(tsk, frame);
>> + ret = unwind_frame(tsk, &frame);
>> if (ret < 0)
>> break;
>> }
>> @@ -195,17 +196,19 @@ 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;
>> -
>> - if (regs)
>> - start_backtrace(&frame, regs->regs[29], regs->pc);
>> - else if (task == current)
>> - start_backtrace(&frame,
>> - (unsigned long)__builtin_frame_address(1),
>> - (unsigned long)__builtin_return_address(0));
>> - else
>> - start_backtrace(&frame, thread_saved_fp(task),
>> - thread_saved_pc(task));
>> -
>> - walk_stackframe(task, &frame, consume_entry, cookie);
>> + unsigned long fp, pc;
>> +
>> + if (regs) {
>> + fp = regs->regs[29];
>> + pc = regs->pc;
>> + } else if (task == current) {
>> + /* Skip arch_stack_walk() in the stack trace. */
>> + fp = (unsigned long)__builtin_frame_address(1);
>> + pc = (unsigned long)__builtin_return_address(0);
>> + } else {
>> + /* Caller guarantees that the task is not running. */
>> + fp = thread_saved_fp(task);
>> + pc = thread_saved_pc(task);
>> + }
>> + walk_stackframe(task, fp, pc, consume_entry, cookie);
>
> I'd prefer to leave this as-is. The new and old structure are largely
> equivalent, so we haven't made this any simpler, but we have added more
> arguments to walk_stackframe().
>
This is just to simplify things when we eventually add arch_stack_walk_reliable().
That is all. All of the unwinding is done by a single unwinding function and
there are two consumers of that unwinding function - arch_stack_walk() and
arch_stack_walk_reliable().
> One thing I *would* like to do is move tsk into strcut stackframe, so we only
> need to pass that around, which'll make it easier to refactor the core unwind
> logic.
>
Will do this in the next version.
Thanks,
Madhavan
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe()
2021-11-30 17:13 ` Madhavan T. Venkataraman
@ 2021-11-30 18:29 ` Mark Rutland
2021-11-30 20:29 ` Madhavan T. Venkataraman
0 siblings, 1 reply; 21+ messages in thread
From: Mark Rutland @ 2021-11-30 18:29 UTC (permalink / raw)
To: Madhavan T. Venkataraman
Cc: broonie, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
On Tue, Nov 30, 2021 at 11:13:28AM -0600, Madhavan T. Venkataraman wrote:
> On 11/30/21 9:05 AM, Mark Rutland wrote:
> > On Tue, Nov 23, 2021 at 01:37:19PM -0600, madvenka@linux.microsoft.com wrote:
> >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> >>
> >> Currently, arch_stack_walk() calls start_backtrace() and walk_stackframe()
> >> separately. There is no need to do that. Instead, call start_backtrace()
> >> from within walk_stackframe(). In other words, walk_stackframe() is the only
> >> unwind function a consumer needs to call.
> >> @@ -143,15 +140,19 @@ static int notrace unwind_frame(struct task_struct *tsk,
> >> NOKPROBE_SYMBOL(unwind_frame);
> >>
> >> static void notrace walk_stackframe(struct task_struct *tsk,
> >> - struct stackframe *frame,
> >> + unsigned long fp, unsigned long pc,
> >> bool (*fn)(void *, unsigned long), void *data)
> >> {
> >> + struct stackframe frame;
> >> +
> >> + start_backtrace(&frame, fp, pc);
> >> +
> >> while (1) {
> >> int ret;
> >>
> >> - if (!fn(data, frame->pc))
> >> + if (!fn(data, frame.pc))
> >> break;
> >> - ret = unwind_frame(tsk, frame);
> >> + ret = unwind_frame(tsk, &frame);
> >> if (ret < 0)
> >> break;
> >> }
> >> @@ -195,17 +196,19 @@ 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;
> >> -
> >> - if (regs)
> >> - start_backtrace(&frame, regs->regs[29], regs->pc);
> >> - else if (task == current)
> >> - start_backtrace(&frame,
> >> - (unsigned long)__builtin_frame_address(1),
> >> - (unsigned long)__builtin_return_address(0));
> >> - else
> >> - start_backtrace(&frame, thread_saved_fp(task),
> >> - thread_saved_pc(task));
> >> -
> >> - walk_stackframe(task, &frame, consume_entry, cookie);
> >> + unsigned long fp, pc;
> >> +
> >> + if (regs) {
> >> + fp = regs->regs[29];
> >> + pc = regs->pc;
> >> + } else if (task == current) {
> >> + /* Skip arch_stack_walk() in the stack trace. */
> >> + fp = (unsigned long)__builtin_frame_address(1);
> >> + pc = (unsigned long)__builtin_return_address(0);
> >> + } else {
> >> + /* Caller guarantees that the task is not running. */
> >> + fp = thread_saved_fp(task);
> >> + pc = thread_saved_pc(task);
> >> + }
> >> + walk_stackframe(task, fp, pc, consume_entry, cookie);
> >
> > I'd prefer to leave this as-is. The new and old structure are largely
> > equivalent, so we haven't made this any simpler, but we have added more
> > arguments to walk_stackframe().
> >
>
> This is just to simplify things when we eventually add arch_stack_walk_reliable().
> That is all. All of the unwinding is done by a single unwinding function and
> there are two consumers of that unwinding function - arch_stack_walk() and
> arch_stack_walk_reliable().
I understand the theory, but I don't think that moving the start_backtrace()
call actually simplifies this in a meaningful way, and I think it'll make it
harder for us to make more meaningful simplifications later on.
As of patch 4 of this series, we'll have:
| noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
| void *cookie, struct task_struct *task,
| struct pt_regs *regs)
| {
| unsigned long fp, pc;
|
| if (regs) {
| fp = regs->regs[29];
| pc = regs->pc;
| } else if (task == current) {
| /* Skip arch_stack_walk() in the stack trace. */
| fp = (unsigned long)__builtin_frame_address(1);
| pc = (unsigned long)__builtin_return_address(0);
| } else {
| /* Caller guarantees that the task is not running. */
| fp = thread_saved_fp(task);
| pc = thread_saved_pc(task);
| }
| walk_stackframe(task, fp, pc, consume_entry, cookie);
| }
|
| noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn,
| void *cookie,
| struct task_struct *task)
| {
| unsigned long fp, pc;
|
| if (task == current) {
| /* Skip arch_stack_walk_reliable() in the stack trace. */
| fp = (unsigned long)__builtin_frame_address(1);
| pc = (unsigned long)__builtin_return_address(0);
| } else {
| /* Caller guarantees that the task is not running. */
| fp = thread_saved_fp(task);
| pc = thread_saved_pc(task);
| }
| if (unwind(task, fp, pc, consume_fn, cookie))
| return 0;
| return -EINVAL;
| }
Which I do not think is substantially simpler than the naive extrapolation from
what we currently have, e.g.
| 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;
|
| if (regs) {
| unwind_init(&frame, regs->regs[29], regs->pc)
| } else if (task == current) {
| unwind_init(&frame, __builtin_frame_address(1),
| __builtin_return_address(0));
| } else {
| unwind_init(&frame, thread_saved_fp(task),
| thread_saved_pc(task);
| }
| walk_stackframe(task, &frame, consume_entry, cookie);
| }
|
| noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn,
| void *cookie,
| struct task_struct *task)
| {
| struct stackframe frame;
|
| if (task == current) {
| unwind_init(&frame, __builtin_frame_address(1),
| __builtin_return_address(0));
| } else {
| unwind_init(&frame, thread_saved_fp(task),
| thread_saved_pc(task);
| }
| if (unwind(task, &frame, consume_fn, cookie))
| return 0;
| return -EINVAL;
| }
Further, I think we can factor this in a different way to reduce the
duplication:
| /*
| * TODO: document requirements here
| */
| static inline void unwind_init_from_current_regs(struct stackframe *frame,
| struct pt_regs *regs)
| {
| unwind_init(frame, regs->regs[29], regs->pc);
| }
|
| /*
| * TODO: document requirements here
| */
| static inline void unwind_init_from_blocked_task(struct stackframe *frame,
| struct task_struct *tsk)
| {
| unwind_init(&frame, thread_saved_fp(task),
| thread_saved_pc(task));
| }
|
| /*
| * 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_from_caller(struct stackframe *frame)
| {
| unwind_init(frame, __builtin_frame_address(1),
| __builtin_return_address(0));
| }
|
| 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;
|
| if (regs)
| unwind_init_current_regs(&frame, regs);
| else if (task == current)
| unwind_init_from_caller(&frame);
| else
| unwind_init_blocked_task(&frame, task);
|
| unwind(task, &frame, consume_entry, cookie);
| }
|
| noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn,
| void *cookie,
| struct task_struct *task)
| {
| struct stackframe frame;
|
| if (task == current)
| unwind_init_from_caller(&frame);
| else
| unwind_init_from_blocked_task(&frame, task);
|
| if (unwind(task, &frame, consume_fn, cookie))
| return 0;
| return -EINVAL;
| }
... which minimizes the duplication and allows us to add specialized
initialization for each case if necessary, which I believe we will need in
future to make unwinding across exception boundaries (such as when starting
with regs) more useful.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe()
2021-11-30 18:29 ` Mark Rutland
@ 2021-11-30 20:29 ` Madhavan T. Venkataraman
2021-12-10 4:13 ` Madhavan T. Venkataraman
0 siblings, 1 reply; 21+ messages in thread
From: Madhavan T. Venkataraman @ 2021-11-30 20:29 UTC (permalink / raw)
To: Mark Rutland
Cc: broonie, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
On 11/30/21 12:29 PM, Mark Rutland wrote:
> On Tue, Nov 30, 2021 at 11:13:28AM -0600, Madhavan T. Venkataraman wrote:
>> On 11/30/21 9:05 AM, Mark Rutland wrote:
>>> On Tue, Nov 23, 2021 at 01:37:19PM -0600, madvenka@linux.microsoft.com wrote:
>>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>>>
>>>> Currently, arch_stack_walk() calls start_backtrace() and walk_stackframe()
>>>> separately. There is no need to do that. Instead, call start_backtrace()
>>>> from within walk_stackframe(). In other words, walk_stackframe() is the only
>>>> unwind function a consumer needs to call.
>
>>>> @@ -143,15 +140,19 @@ static int notrace unwind_frame(struct task_struct *tsk,
>>>> NOKPROBE_SYMBOL(unwind_frame);
>>>>
>>>> static void notrace walk_stackframe(struct task_struct *tsk,
>>>> - struct stackframe *frame,
>>>> + unsigned long fp, unsigned long pc,
>>>> bool (*fn)(void *, unsigned long), void *data)
>>>> {
>>>> + struct stackframe frame;
>>>> +
>>>> + start_backtrace(&frame, fp, pc);
>>>> +
>>>> while (1) {
>>>> int ret;
>>>>
>>>> - if (!fn(data, frame->pc))
>>>> + if (!fn(data, frame.pc))
>>>> break;
>>>> - ret = unwind_frame(tsk, frame);
>>>> + ret = unwind_frame(tsk, &frame);
>>>> if (ret < 0)
>>>> break;
>>>> }
>>>> @@ -195,17 +196,19 @@ 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;
>>>> -
>>>> - if (regs)
>>>> - start_backtrace(&frame, regs->regs[29], regs->pc);
>>>> - else if (task == current)
>>>> - start_backtrace(&frame,
>>>> - (unsigned long)__builtin_frame_address(1),
>>>> - (unsigned long)__builtin_return_address(0));
>>>> - else
>>>> - start_backtrace(&frame, thread_saved_fp(task),
>>>> - thread_saved_pc(task));
>>>> -
>>>> - walk_stackframe(task, &frame, consume_entry, cookie);
>>>> + unsigned long fp, pc;
>>>> +
>>>> + if (regs) {
>>>> + fp = regs->regs[29];
>>>> + pc = regs->pc;
>>>> + } else if (task == current) {
>>>> + /* Skip arch_stack_walk() in the stack trace. */
>>>> + fp = (unsigned long)__builtin_frame_address(1);
>>>> + pc = (unsigned long)__builtin_return_address(0);
>>>> + } else {
>>>> + /* Caller guarantees that the task is not running. */
>>>> + fp = thread_saved_fp(task);
>>>> + pc = thread_saved_pc(task);
>>>> + }
>>>> + walk_stackframe(task, fp, pc, consume_entry, cookie);
>>>
>>> I'd prefer to leave this as-is. The new and old structure are largely
>>> equivalent, so we haven't made this any simpler, but we have added more
>>> arguments to walk_stackframe().
>>>
>>
>> This is just to simplify things when we eventually add arch_stack_walk_reliable().
>> That is all. All of the unwinding is done by a single unwinding function and
>> there are two consumers of that unwinding function - arch_stack_walk() and
>> arch_stack_walk_reliable().
>
> I understand the theory, but I don't think that moving the start_backtrace()
> call actually simplifies this in a meaningful way, and I think it'll make it
> harder for us to make more meaningful simplifications later on.
>
> As of patch 4 of this series, we'll have:
>
> | noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
> | void *cookie, struct task_struct *task,
> | struct pt_regs *regs)
> | {
> | unsigned long fp, pc;
> |
> | if (regs) {
> | fp = regs->regs[29];
> | pc = regs->pc;
> | } else if (task == current) {
> | /* Skip arch_stack_walk() in the stack trace. */
> | fp = (unsigned long)__builtin_frame_address(1);
> | pc = (unsigned long)__builtin_return_address(0);
> | } else {
> | /* Caller guarantees that the task is not running. */
> | fp = thread_saved_fp(task);
> | pc = thread_saved_pc(task);
> | }
> | walk_stackframe(task, fp, pc, consume_entry, cookie);
> | }
> |
> | noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn,
> | void *cookie,
> | struct task_struct *task)
> | {
> | unsigned long fp, pc;
> |
> | if (task == current) {
> | /* Skip arch_stack_walk_reliable() in the stack trace. */
> | fp = (unsigned long)__builtin_frame_address(1);
> | pc = (unsigned long)__builtin_return_address(0);
> | } else {
> | /* Caller guarantees that the task is not running. */
> | fp = thread_saved_fp(task);
> | pc = thread_saved_pc(task);
> | }
> | if (unwind(task, fp, pc, consume_fn, cookie))
> | return 0;
> | return -EINVAL;
> | }
>
> Which I do not think is substantially simpler than the naive extrapolation from
> what we currently have, e.g.
>
> | 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;
> |
> | if (regs) {
> | unwind_init(&frame, regs->regs[29], regs->pc)
> | } else if (task == current) {
> | unwind_init(&frame, __builtin_frame_address(1),
> | __builtin_return_address(0));
> | } else {
> | unwind_init(&frame, thread_saved_fp(task),
> | thread_saved_pc(task);
> | }
> | walk_stackframe(task, &frame, consume_entry, cookie);
> | }
> |
> | noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn,
> | void *cookie,
> | struct task_struct *task)
> | {
> | struct stackframe frame;
> |
> | if (task == current) {
> | unwind_init(&frame, __builtin_frame_address(1),
> | __builtin_return_address(0));
> | } else {
> | unwind_init(&frame, thread_saved_fp(task),
> | thread_saved_pc(task);
> | }
> | if (unwind(task, &frame, consume_fn, cookie))
> | return 0;
> | return -EINVAL;
> | }
>
> Further, I think we can factor this in a different way to reduce the
> duplication:
>
> | /*
> | * TODO: document requirements here
> | */
> | static inline void unwind_init_from_current_regs(struct stackframe *frame,
> | struct pt_regs *regs)
> | {
> | unwind_init(frame, regs->regs[29], regs->pc);
> | }
> |
> | /*
> | * TODO: document requirements here
> | */
> | static inline void unwind_init_from_blocked_task(struct stackframe *frame,
> | struct task_struct *tsk)
> | {
> | unwind_init(&frame, thread_saved_fp(task),
> | thread_saved_pc(task));
> | }
> |
> | /*
> | * 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_from_caller(struct stackframe *frame)
> | {
> | unwind_init(frame, __builtin_frame_address(1),
> | __builtin_return_address(0));
> | }
> |
> | 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;
> |
> | if (regs)
> | unwind_init_current_regs(&frame, regs);
> | else if (task == current)
> | unwind_init_from_caller(&frame);
> | else
> | unwind_init_blocked_task(&frame, task);
> |
> | unwind(task, &frame, consume_entry, cookie);
> | }
> |
> | noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn,
> | void *cookie,
> | struct task_struct *task)
> | {
> | struct stackframe frame;
> |
> | if (task == current)
> | unwind_init_from_caller(&frame);
> | else
> | unwind_init_from_blocked_task(&frame, task);
> |
> | if (unwind(task, &frame, consume_fn, cookie))
> | return 0;
> | return -EINVAL;
> | }
>
> ... which minimizes the duplication and allows us to add specialized
> initialization for each case if necessary, which I believe we will need in
> future to make unwinding across exception boundaries (such as when starting
> with regs) more useful.
>
> Thanks,
> Mark.
>
OK. I don't mind doing it this way.
Thanks.
Madhavan
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe()
2021-11-30 20:29 ` Madhavan T. Venkataraman
@ 2021-12-10 4:13 ` Madhavan T. Venkataraman
0 siblings, 0 replies; 21+ messages in thread
From: Madhavan T. Venkataraman @ 2021-12-10 4:13 UTC (permalink / raw)
To: Mark Rutland
Cc: broonie, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
Hey Mark,
Do you have any comments on the rest of the series? I am working on the next version of the patchset.
If you have any other comments, I will wait.
Thanks.
Madhavan
On 11/30/21 2:29 PM, Madhavan T. Venkataraman wrote:
>
>
> On 11/30/21 12:29 PM, Mark Rutland wrote:
>> On Tue, Nov 30, 2021 at 11:13:28AM -0600, Madhavan T. Venkataraman wrote:
>>> On 11/30/21 9:05 AM, Mark Rutland wrote:
>>>> On Tue, Nov 23, 2021 at 01:37:19PM -0600, madvenka@linux.microsoft.com wrote:
>>>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>>>>
>>>>> Currently, arch_stack_walk() calls start_backtrace() and walk_stackframe()
>>>>> separately. There is no need to do that. Instead, call start_backtrace()
>>>>> from within walk_stackframe(). In other words, walk_stackframe() is the only
>>>>> unwind function a consumer needs to call.
>>
>>>>> @@ -143,15 +140,19 @@ static int notrace unwind_frame(struct task_struct *tsk,
>>>>> NOKPROBE_SYMBOL(unwind_frame);
>>>>>
>>>>> static void notrace walk_stackframe(struct task_struct *tsk,
>>>>> - struct stackframe *frame,
>>>>> + unsigned long fp, unsigned long pc,
>>>>> bool (*fn)(void *, unsigned long), void *data)
>>>>> {
>>>>> + struct stackframe frame;
>>>>> +
>>>>> + start_backtrace(&frame, fp, pc);
>>>>> +
>>>>> while (1) {
>>>>> int ret;
>>>>>
>>>>> - if (!fn(data, frame->pc))
>>>>> + if (!fn(data, frame.pc))
>>>>> break;
>>>>> - ret = unwind_frame(tsk, frame);
>>>>> + ret = unwind_frame(tsk, &frame);
>>>>> if (ret < 0)
>>>>> break;
>>>>> }
>>>>> @@ -195,17 +196,19 @@ 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;
>>>>> -
>>>>> - if (regs)
>>>>> - start_backtrace(&frame, regs->regs[29], regs->pc);
>>>>> - else if (task == current)
>>>>> - start_backtrace(&frame,
>>>>> - (unsigned long)__builtin_frame_address(1),
>>>>> - (unsigned long)__builtin_return_address(0));
>>>>> - else
>>>>> - start_backtrace(&frame, thread_saved_fp(task),
>>>>> - thread_saved_pc(task));
>>>>> -
>>>>> - walk_stackframe(task, &frame, consume_entry, cookie);
>>>>> + unsigned long fp, pc;
>>>>> +
>>>>> + if (regs) {
>>>>> + fp = regs->regs[29];
>>>>> + pc = regs->pc;
>>>>> + } else if (task == current) {
>>>>> + /* Skip arch_stack_walk() in the stack trace. */
>>>>> + fp = (unsigned long)__builtin_frame_address(1);
>>>>> + pc = (unsigned long)__builtin_return_address(0);
>>>>> + } else {
>>>>> + /* Caller guarantees that the task is not running. */
>>>>> + fp = thread_saved_fp(task);
>>>>> + pc = thread_saved_pc(task);
>>>>> + }
>>>>> + walk_stackframe(task, fp, pc, consume_entry, cookie);
>>>>
>>>> I'd prefer to leave this as-is. The new and old structure are largely
>>>> equivalent, so we haven't made this any simpler, but we have added more
>>>> arguments to walk_stackframe().
>>>>
>>>
>>> This is just to simplify things when we eventually add arch_stack_walk_reliable().
>>> That is all. All of the unwinding is done by a single unwinding function and
>>> there are two consumers of that unwinding function - arch_stack_walk() and
>>> arch_stack_walk_reliable().
>>
>> I understand the theory, but I don't think that moving the start_backtrace()
>> call actually simplifies this in a meaningful way, and I think it'll make it
>> harder for us to make more meaningful simplifications later on.
>>
>> As of patch 4 of this series, we'll have:
>>
>> | noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>> | void *cookie, struct task_struct *task,
>> | struct pt_regs *regs)
>> | {
>> | unsigned long fp, pc;
>> |
>> | if (regs) {
>> | fp = regs->regs[29];
>> | pc = regs->pc;
>> | } else if (task == current) {
>> | /* Skip arch_stack_walk() in the stack trace. */
>> | fp = (unsigned long)__builtin_frame_address(1);
>> | pc = (unsigned long)__builtin_return_address(0);
>> | } else {
>> | /* Caller guarantees that the task is not running. */
>> | fp = thread_saved_fp(task);
>> | pc = thread_saved_pc(task);
>> | }
>> | walk_stackframe(task, fp, pc, consume_entry, cookie);
>> | }
>> |
>> | noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn,
>> | void *cookie,
>> | struct task_struct *task)
>> | {
>> | unsigned long fp, pc;
>> |
>> | if (task == current) {
>> | /* Skip arch_stack_walk_reliable() in the stack trace. */
>> | fp = (unsigned long)__builtin_frame_address(1);
>> | pc = (unsigned long)__builtin_return_address(0);
>> | } else {
>> | /* Caller guarantees that the task is not running. */
>> | fp = thread_saved_fp(task);
>> | pc = thread_saved_pc(task);
>> | }
>> | if (unwind(task, fp, pc, consume_fn, cookie))
>> | return 0;
>> | return -EINVAL;
>> | }
>>
>> Which I do not think is substantially simpler than the naive extrapolation from
>> what we currently have, e.g.
>>
>> | 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;
>> |
>> | if (regs) {
>> | unwind_init(&frame, regs->regs[29], regs->pc)
>> | } else if (task == current) {
>> | unwind_init(&frame, __builtin_frame_address(1),
>> | __builtin_return_address(0));
>> | } else {
>> | unwind_init(&frame, thread_saved_fp(task),
>> | thread_saved_pc(task);
>> | }
>> | walk_stackframe(task, &frame, consume_entry, cookie);
>> | }
>> |
>> | noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn,
>> | void *cookie,
>> | struct task_struct *task)
>> | {
>> | struct stackframe frame;
>> |
>> | if (task == current) {
>> | unwind_init(&frame, __builtin_frame_address(1),
>> | __builtin_return_address(0));
>> | } else {
>> | unwind_init(&frame, thread_saved_fp(task),
>> | thread_saved_pc(task);
>> | }
>> | if (unwind(task, &frame, consume_fn, cookie))
>> | return 0;
>> | return -EINVAL;
>> | }
>>
>> Further, I think we can factor this in a different way to reduce the
>> duplication:
>>
>> | /*
>> | * TODO: document requirements here
>> | */
>> | static inline void unwind_init_from_current_regs(struct stackframe *frame,
>> | struct pt_regs *regs)
>> | {
>> | unwind_init(frame, regs->regs[29], regs->pc);
>> | }
>> |
>> | /*
>> | * TODO: document requirements here
>> | */
>> | static inline void unwind_init_from_blocked_task(struct stackframe *frame,
>> | struct task_struct *tsk)
>> | {
>> | unwind_init(&frame, thread_saved_fp(task),
>> | thread_saved_pc(task));
>> | }
>> |
>> | /*
>> | * 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_from_caller(struct stackframe *frame)
>> | {
>> | unwind_init(frame, __builtin_frame_address(1),
>> | __builtin_return_address(0));
>> | }
>> |
>> | 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;
>> |
>> | if (regs)
>> | unwind_init_current_regs(&frame, regs);
>> | else if (task == current)
>> | unwind_init_from_caller(&frame);
>> | else
>> | unwind_init_blocked_task(&frame, task);
>> |
>> | unwind(task, &frame, consume_entry, cookie);
>> | }
>> |
>> | noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn,
>> | void *cookie,
>> | struct task_struct *task)
>> | {
>> | struct stackframe frame;
>> |
>> | if (task == current)
>> | unwind_init_from_caller(&frame);
>> | else
>> | unwind_init_from_blocked_task(&frame, task);
>> |
>> | if (unwind(task, &frame, consume_fn, cookie))
>> | return 0;
>> | return -EINVAL;
>> | }
>>
>> ... which minimizes the duplication and allows us to add specialized
>> initialization for each case if necessary, which I believe we will need in
>> future to make unwinding across exception boundaries (such as when starting
>> with regs) more useful.
>>
>> Thanks,
>> Mark.
>>
>
> OK. I don't mind doing it this way.
>
> Thanks.
>
> Madhavan
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v11 2/5] arm64: Rename unwinder functions
2021-11-23 19:37 ` [PATCH v11 0/5] arm64: Reorganize the unwinder and implement stack trace reliability checks madvenka
2021-11-23 19:37 ` [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe() madvenka
@ 2021-11-23 19:37 ` madvenka
2021-11-24 17:10 ` Mark Brown
2021-11-30 15:08 ` Mark Rutland
2021-11-23 19:37 ` [PATCH v11 3/5] arm64: Make the unwind loop in unwind() similar to other architectures madvenka
` (2 subsequent siblings)
4 siblings, 2 replies; 21+ messages in thread
From: madvenka @ 2021-11-23 19:37 UTC (permalink / raw)
To: mark.rutland, broonie, jpoimboe, ardb, nobuta.keiya,
sjitindarsingh, catalin.marinas, will, jmorris, linux-arm-kernel,
live-patching, linux-kernel, madvenka
From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
Rename unwinder functions for consistency and better naming.
- Rename start_backtrace() to unwind_start().
- Rename unwind_frame() to unwind_next().
- Rename walk_stackframe() to unwind().
Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
---
arch/arm64/kernel/stacktrace.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 7217c4f63ef7..918852cd2681 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_start(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,27 +137,27 @@ 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,
- unsigned long fp, unsigned long pc,
- bool (*fn)(void *, unsigned long), void *data)
+static void notrace unwind(struct task_struct *tsk,
+ unsigned long fp, unsigned long pc,
+ bool (*fn)(void *, unsigned long), void *data)
{
struct stackframe frame;
- start_backtrace(&frame, fp, pc);
+ unwind_start(&frame, fp, pc);
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)
{
@@ -210,5 +210,5 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
fp = thread_saved_fp(task);
pc = thread_saved_pc(task);
}
- walk_stackframe(task, fp, pc, consume_entry, cookie);
+ unwind(task, fp, pc, consume_entry, cookie);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v11 2/5] arm64: Rename unwinder functions
2021-11-23 19:37 ` [PATCH v11 2/5] arm64: Rename unwinder functions madvenka
@ 2021-11-24 17:10 ` Mark Brown
2021-11-30 15:08 ` Mark Rutland
1 sibling, 0 replies; 21+ messages in thread
From: Mark Brown @ 2021-11-24 17:10 UTC (permalink / raw)
To: madvenka
Cc: mark.rutland, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 406 bytes --]
On Tue, Nov 23, 2021 at 01:37:20PM -0600, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>
> Rename unwinder functions for consistency and better naming.
>
> - Rename start_backtrace() to unwind_start().
> - Rename unwind_frame() to unwind_next().
> - Rename walk_stackframe() to unwind().
Reviewed-by: Mark Brown <broonie@kernel.org>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v11 2/5] arm64: Rename unwinder functions
2021-11-23 19:37 ` [PATCH v11 2/5] arm64: Rename unwinder functions madvenka
2021-11-24 17:10 ` Mark Brown
@ 2021-11-30 15:08 ` Mark Rutland
2021-11-30 17:15 ` Madhavan T. Venkataraman
1 sibling, 1 reply; 21+ messages in thread
From: Mark Rutland @ 2021-11-30 15:08 UTC (permalink / raw)
To: madvenka
Cc: broonie, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
On Tue, Nov 23, 2021 at 01:37:20PM -0600, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>
> Rename unwinder functions for consistency and better naming.
>
> - Rename start_backtrace() to unwind_start().
> - Rename unwind_frame() to unwind_next().
> - Rename walk_stackframe() to unwind().
Super trivial, but could we s/unwind_start/unwind_init/ ? That makes it
slightly clearer that it's not performing an unwind step.
Otherwise, this looks good to me.
For the rename:
Acked-by: Mark Rutland <mark.rutland@arm.com>
It's be nice if we could also clean up 'struct stackframe' into 'struct
unwind_state', but that can be a follow-up, and this is fine as it is, modulo
the super trivial comment above.
Thanks,
Mark.
>
> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> ---
> arch/arm64/kernel/stacktrace.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 7217c4f63ef7..918852cd2681 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_start(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,27 +137,27 @@ 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,
> - unsigned long fp, unsigned long pc,
> - bool (*fn)(void *, unsigned long), void *data)
> +static void notrace unwind(struct task_struct *tsk,
> + unsigned long fp, unsigned long pc,
> + bool (*fn)(void *, unsigned long), void *data)
> {
> struct stackframe frame;
>
> - start_backtrace(&frame, fp, pc);
> + unwind_start(&frame, fp, pc);
>
> 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)
> {
> @@ -210,5 +210,5 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
> fp = thread_saved_fp(task);
> pc = thread_saved_pc(task);
> }
> - walk_stackframe(task, fp, pc, consume_entry, cookie);
> + unwind(task, fp, pc, consume_entry, cookie);
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v11 2/5] arm64: Rename unwinder functions
2021-11-30 15:08 ` Mark Rutland
@ 2021-11-30 17:15 ` Madhavan T. Venkataraman
0 siblings, 0 replies; 21+ messages in thread
From: Madhavan T. Venkataraman @ 2021-11-30 17:15 UTC (permalink / raw)
To: Mark Rutland
Cc: broonie, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
On 11/30/21 9:08 AM, Mark Rutland wrote:
> On Tue, Nov 23, 2021 at 01:37:20PM -0600, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> Rename unwinder functions for consistency and better naming.
>>
>> - Rename start_backtrace() to unwind_start().
>> - Rename unwind_frame() to unwind_next().
>> - Rename walk_stackframe() to unwind().
>
> Super trivial, but could we s/unwind_start/unwind_init/ ? That makes it
> slightly clearer that it's not performing an unwind step.
>
> Otherwise, this looks good to me.
>
Will do.
> For the rename:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>
> It's be nice if we could also clean up 'struct stackframe' into 'struct
> unwind_state', but that can be a follow-up, and this is fine as it is, modulo
> the super trivial comment above.
>
I could do this rename as well in the next version if you want. Might as well
get all the renaming done in one shot.
Thanks,
Madhavan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v11 3/5] arm64: Make the unwind loop in unwind() similar to other architectures
2021-11-23 19:37 ` [PATCH v11 0/5] arm64: Reorganize the unwinder and implement stack trace reliability checks madvenka
2021-11-23 19:37 ` [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe() madvenka
2021-11-23 19:37 ` [PATCH v11 2/5] arm64: Rename unwinder functions madvenka
@ 2021-11-23 19:37 ` madvenka
2021-11-25 14:30 ` Mark Brown
2021-11-23 19:37 ` [PATCH v11 4/5] arm64: Introduce stack trace reliability checks in the unwinder madvenka
2021-11-23 19:37 ` [PATCH v11 5/5] arm64: Create a list of SYM_CODE functions, check return PC against list madvenka
4 siblings, 1 reply; 21+ messages in thread
From: madvenka @ 2021-11-23 19:37 UTC (permalink / raw)
To: mark.rutland, broonie, jpoimboe, ardb, nobuta.keiya,
sjitindarsingh, catalin.marinas, will, jmorris, linux-arm-kernel,
live-patching, linux-kernel, madvenka
From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
Change the loop in unwind()
===========================
Change the unwind loop in unwind() to:
unwind_start(&frame, fp, pc);
while (unwind_continue(tsk, &frame, fn, data))
unwind_next(tsk, &frame);
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 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 the stackframe and check the flag in unwind_continue().
Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
---
arch/arm64/include/asm/stacktrace.h | 3 ++
arch/arm64/kernel/stacktrace.c | 75 ++++++++++++++++++-----------
2 files changed, 50 insertions(+), 28 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 3a15d376ab36..d838586adef9 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -51,6 +51,8 @@ struct stack_info {
* @kr_cur: When KRETPOLINES is selected, holds the kretprobe instance
* associated with the most recently encountered replacement lr
* value.
+ *
+ * @failed: Unwind failed.
*/
struct stackframe {
unsigned long fp;
@@ -61,6 +63,7 @@ struct stackframe {
#ifdef CONFIG_KRETPROBES
struct llist_node *kr_cur;
#endif
+ 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 918852cd2681..3b670ab1f0e9 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -54,6 +54,7 @@ static void unwind_start(struct stackframe *frame, unsigned long fp,
bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
frame->prev_fp = 0;
frame->prev_type = STACK_TYPE_UNKNOWN;
+ frame->failed = false;
}
/*
@@ -63,24 +64,26 @@ static void unwind_start(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_next(struct task_struct *tsk,
- struct stackframe *frame)
+static void notrace unwind_next(struct task_struct *tsk,
+ struct stackframe *frame)
{
unsigned long fp = frame->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) {
+ frame->failed = true;
+ return;
+ }
- if (!on_accessible_stack(tsk, fp, 16, &info))
- return -EINVAL;
+ if (!on_accessible_stack(tsk, fp, 16, &info)) {
+ frame->failed = true;
+ return;
+ }
- if (test_bit(info.type, frame->stacks_done))
- return -EINVAL;
+ if (test_bit(info.type, frame->stacks_done)) {
+ frame->failed = true;
+ return;
+ }
/*
* As stacks grow downward, any valid record on the same stack must be
@@ -96,8 +99,10 @@ static int notrace unwind_next(struct task_struct *tsk,
* stack.
*/
if (info.type == frame->prev_type) {
- if (fp <= frame->prev_fp)
- return -EINVAL;
+ if (fp <= frame->prev_fp) {
+ frame->failed = true;
+ return;
+ }
} else {
set_bit(frame->prev_type, frame->stacks_done);
}
@@ -125,8 +130,10 @@ static int notrace unwind_next(struct task_struct *tsk,
*/
orig_pc = ftrace_graph_ret_addr(tsk, NULL, frame->pc,
(void *)frame->fp);
- if (WARN_ON_ONCE(frame->pc == orig_pc))
- return -EINVAL;
+ if (WARN_ON_ONCE(frame->pc == orig_pc)) {
+ frame->failed = true;
+ return;
+ }
frame->pc = orig_pc;
}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
@@ -134,11 +141,31 @@ static int notrace unwind_next(struct task_struct *tsk,
if (is_kretprobe_trampoline(frame->pc))
frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);
#endif
-
- return 0;
}
NOKPROBE_SYMBOL(unwind_next);
+static bool unwind_continue(struct task_struct *task,
+ struct stackframe *frame,
+ stack_trace_consume_fn consume_entry, void *cookie)
+{
+ if (frame->failed) {
+ /* PC is suspect. Cannot consume it. */
+ return false;
+ }
+
+ if (!consume_entry(cookie, frame->pc)) {
+ /* Caller terminated the unwind. */
+ frame->failed = true;
+ return false;
+ }
+
+ if (frame->fp == (unsigned long)task_pt_regs(task)->stackframe) {
+ /* Final frame; nothing to unwind */
+ return false;
+ }
+ return true;
+}
+
static void notrace unwind(struct task_struct *tsk,
unsigned long fp, unsigned long pc,
bool (*fn)(void *, unsigned long), void *data)
@@ -146,16 +173,8 @@ static void notrace unwind(struct task_struct *tsk,
struct stackframe frame;
unwind_start(&frame, fp, pc);
-
- while (1) {
- int ret;
-
- if (!fn(data, frame.pc))
- break;
- ret = unwind_next(tsk, &frame);
- if (ret < 0)
- break;
- }
+ while (unwind_continue(tsk, &frame, fn, data))
+ unwind_next(tsk, &frame);
}
NOKPROBE_SYMBOL(unwind);
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v11 3/5] arm64: Make the unwind loop in unwind() similar to other architectures
2021-11-23 19:37 ` [PATCH v11 3/5] arm64: Make the unwind loop in unwind() similar to other architectures madvenka
@ 2021-11-25 14:30 ` Mark Brown
0 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2021-11-25 14:30 UTC (permalink / raw)
To: madvenka
Cc: mark.rutland, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 686 bytes --]
On Tue, Nov 23, 2021 at 01:37:21PM -0600, madvenka@linux.microsoft.com wrote:
> unwind_start(&frame, fp, pc);
> while (unwind_continue(tsk, &frame, fn, data))
> unwind_next(tsk, &frame);
Other architectures seem to call their unwind_next() unwind_next_frame()
instead, and use a function unwind_done() rather than unwind_continue().
I appreciate that's actually a change carried through from one of the
earlier patches but might be worth considering. I don't really *mind*
that, though if there's any work on pulling more code out of the
architecture into the generic code it'll need revisiting in at least
some of the architectures.
Reviwed-by: Mark Brown <broonie@kernel.org>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v11 4/5] arm64: Introduce stack trace reliability checks in the unwinder
2021-11-23 19:37 ` [PATCH v11 0/5] arm64: Reorganize the unwinder and implement stack trace reliability checks madvenka
` (2 preceding siblings ...)
2021-11-23 19:37 ` [PATCH v11 3/5] arm64: Make the unwind loop in unwind() similar to other architectures madvenka
@ 2021-11-23 19:37 ` madvenka
2021-11-25 14:56 ` Mark Brown
2021-11-23 19:37 ` [PATCH v11 5/5] arm64: Create a list of SYM_CODE functions, check return PC against list madvenka
4 siblings, 1 reply; 21+ messages in thread
From: madvenka @ 2021-11-23 19:37 UTC (permalink / raw)
To: mark.rutland, broonie, jpoimboe, ardb, nobuta.keiya,
sjitindarsingh, catalin.marinas, will, jmorris, linux-arm-kernel,
live-patching, linux-kernel, madvenka
From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
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, that is, in unwind_start()
and unwind_next().
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.
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 <madvenka@linux.microsoft.com>
---
arch/arm64/include/asm/stacktrace.h | 3 ++
arch/arm64/kernel/stacktrace.c | 59 +++++++++++++++++++++++++++--
2 files changed, 58 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index d838586adef9..7143e80c3d96 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -53,6 +53,8 @@ struct stack_info {
* value.
*
* @failed: Unwind failed.
+ *
+ * @reliable: Stack trace is reliable.
*/
struct stackframe {
unsigned long fp;
@@ -64,6 +66,7 @@ struct stackframe {
struct llist_node *kr_cur;
#endif
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 3b670ab1f0e9..77eb00e45558 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -18,6 +18,26 @@
#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 task_struct *task,
+ struct stackframe *frame)
+{
+ if (frame->fp == (unsigned long)task_pt_regs(task)->stackframe) {
+ /* 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(frame->pc))
+ frame->reliable = false;
+}
+
/*
* AArch64 PCS assigns the frame pointer to x29.
*
@@ -33,8 +53,9 @@
*/
-static void unwind_start(struct stackframe *frame, unsigned long fp,
- unsigned long pc)
+static void unwind_start(struct task_struct *task,
+ struct stackframe *frame,
+ unsigned long fp, unsigned long pc)
{
frame->fp = fp;
frame->pc = pc;
@@ -55,6 +76,8 @@ static void unwind_start(struct stackframe *frame, unsigned long fp,
frame->prev_fp = 0;
frame->prev_type = STACK_TYPE_UNKNOWN;
frame->failed = false;
+ frame->reliable = true;
+ unwind_check_reliability(task, frame);
}
/*
@@ -141,6 +164,7 @@ static void notrace unwind_next(struct task_struct *tsk,
if (is_kretprobe_trampoline(frame->pc))
frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);
#endif
+ unwind_check_reliability(tsk, frame);
}
NOKPROBE_SYMBOL(unwind_next);
@@ -166,15 +190,16 @@ static bool unwind_continue(struct task_struct *task,
return true;
}
-static void notrace unwind(struct task_struct *tsk,
+static bool notrace unwind(struct task_struct *tsk,
unsigned long fp, unsigned long pc,
bool (*fn)(void *, unsigned long), void *data)
{
struct stackframe frame;
- unwind_start(&frame, fp, pc);
+ unwind_start(tsk, &frame, fp, pc);
while (unwind_continue(tsk, &frame, fn, data))
unwind_next(tsk, &frame);
+ return !frame.failed && frame.reliable;
}
NOKPROBE_SYMBOL(unwind);
@@ -231,3 +256,29 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
}
unwind(task, fp, pc, consume_entry, cookie);
}
+
+/*
+ * 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)
+{
+ unsigned long fp, pc;
+
+ if (task == current) {
+ /* Skip arch_stack_walk_reliable() in the stack trace. */
+ fp = (unsigned long)__builtin_frame_address(1);
+ pc = (unsigned long)__builtin_return_address(0);
+ } else {
+ /* Caller guarantees that the task is not running. */
+ fp = thread_saved_fp(task);
+ pc = thread_saved_pc(task);
+ }
+ if (unwind(task, fp, pc, consume_fn, cookie))
+ return 0;
+ return -EINVAL;
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v11 4/5] arm64: Introduce stack trace reliability checks in the unwinder
2021-11-23 19:37 ` [PATCH v11 4/5] arm64: Introduce stack trace reliability checks in the unwinder madvenka
@ 2021-11-25 14:56 ` Mark Brown
2021-11-25 16:59 ` Madhavan T. Venkataraman
0 siblings, 1 reply; 21+ messages in thread
From: Mark Brown @ 2021-11-25 14:56 UTC (permalink / raw)
To: madvenka
Cc: mark.rutland, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]
On Tue, Nov 23, 2021 at 01:37:22PM -0600, madvenka@linux.microsoft.com wrote:
> 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.
Probably also worth noting that this doesn't select
HAVE_RELIABLE_STACKTRACE which is what any actual users are going to use
to identify if the architecture has the feature. I would have been
tempted to add arch_stack_walk() as a separate patch but equally having
the user code there (even if it itself can't yet be used...) helps with
reviewing the actual unwinder so I don't mind.
> +static void unwind_check_reliability(struct task_struct *task,
> + struct stackframe *frame)
> +{
> + if (frame->fp == (unsigned long)task_pt_regs(task)->stackframe) {
> + /* Final frame; no more unwind, no need to check reliability */
> + return;
> + }
If the unwinder carries on for some reason (the code for that is
elsewhere and may be updated separately...) then this will start
checking again. I'm not sure if this is a *problem* as such but the
thing about this being the final frame coupled with not actually
explicitly stopping the unwind here makes me think this should at least
be clearer, the comment begs the question about what happens if
something decides it is not in fact the final frame.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v11 4/5] arm64: Introduce stack trace reliability checks in the unwinder
2021-11-25 14:56 ` Mark Brown
@ 2021-11-25 16:59 ` Madhavan T. Venkataraman
2021-11-26 13:29 ` Mark Brown
0 siblings, 1 reply; 21+ messages in thread
From: Madhavan T. Venkataraman @ 2021-11-25 16:59 UTC (permalink / raw)
To: Mark Brown
Cc: mark.rutland, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
On 11/25/21 8:56 AM, Mark Brown wrote:
> On Tue, Nov 23, 2021 at 01:37:22PM -0600, madvenka@linux.microsoft.com wrote:
>
>> 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.
>
> Probably also worth noting that this doesn't select
> HAVE_RELIABLE_STACKTRACE which is what any actual users are going to use
> to identify if the architecture has the feature. I would have been
> tempted to add arch_stack_walk() as a separate patch but equally having
> the user code there (even if it itself can't yet be used...) helps with
> reviewing the actual unwinder so I don't mind.
>
I did not select HAVE_RELIABLE_STACKTRACE just in case we think that some
more reliability checks need to be added. But if reviewers agree
that this patch series contains all the reliability checks we need, I
will add a patch to select HAVE_RELIABLE_STACKTRACE to the series.
>> +static void unwind_check_reliability(struct task_struct *task,
>> + struct stackframe *frame)
>> +{
>> + if (frame->fp == (unsigned long)task_pt_regs(task)->stackframe) {
>> + /* Final frame; no more unwind, no need to check reliability */
>> + return;
>> + }
>
> If the unwinder carries on for some reason (the code for that is
> elsewhere and may be updated separately...) then this will start
> checking again. I'm not sure if this is a *problem* as such but the
> thing about this being the final frame coupled with not actually
> explicitly stopping the unwind here makes me think this should at least
> be clearer, the comment begs the question about what happens if
> something decides it is not in fact the final frame.
>
I can address this by adding an explicit comment to that effect.
For example, define a separate function to check for the final frame:
/*
* Check if this is the final frame. Unwind must stop at the final
* frame.
*/
static inline bool unwind_is_final_frame(struct task_struct *task,
struct stackframe *frame)
{
return frame->fp == (unsigned long)task_pt_regs(task)->stackframe;
}
Then, use this function in unwind_check_reliability() and unwind_continue().
Is this acceptable?
Madhavan
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v11 4/5] arm64: Introduce stack trace reliability checks in the unwinder
2021-11-25 16:59 ` Madhavan T. Venkataraman
@ 2021-11-26 13:29 ` Mark Brown
2021-11-26 17:23 ` Madhavan T. Venkataraman
0 siblings, 1 reply; 21+ messages in thread
From: Mark Brown @ 2021-11-26 13:29 UTC (permalink / raw)
To: Madhavan T. Venkataraman
Cc: mark.rutland, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2728 bytes --]
On Thu, Nov 25, 2021 at 10:59:27AM -0600, Madhavan T. Venkataraman wrote:
> On 11/25/21 8:56 AM, Mark Brown wrote:
> > On Tue, Nov 23, 2021 at 01:37:22PM -0600, madvenka@linux.microsoft.com wrote:
> > Probably also worth noting that this doesn't select
> > HAVE_RELIABLE_STACKTRACE which is what any actual users are going to use
> > to identify if the architecture has the feature. I would have been
> > tempted to add arch_stack_walk() as a separate patch but equally having
> > the user code there (even if it itself can't yet be used...) helps with
> > reviewing the actual unwinder so I don't mind.
> I did not select HAVE_RELIABLE_STACKTRACE just in case we think that some
> more reliability checks need to be added. But if reviewers agree
> that this patch series contains all the reliability checks we need, I
> will add a patch to select HAVE_RELIABLE_STACKTRACE to the series.
I agree that more checks probably need to be added, might be worth
throwing that patch into the end of the series though to provide a place
to discuss what exactly we need. My main thought here was that it's
worth explicitly highlighting in this change that the Kconfig bit isn't
glued up here so reviewers notice that's what's happening.
> >> +static void unwind_check_reliability(struct task_struct *task,
> >> + struct stackframe *frame)
> >> +{
> >> + if (frame->fp == (unsigned long)task_pt_regs(task)->stackframe) {
> >> + /* Final frame; no more unwind, no need to check reliability */
> >> + return;
> >> + }
> > If the unwinder carries on for some reason (the code for that is
> > elsewhere and may be updated separately...) then this will start
> > checking again. I'm not sure if this is a *problem* as such but the
> > thing about this being the final frame coupled with not actually
> > explicitly stopping the unwind here makes me think this should at least
> > be clearer, the comment begs the question about what happens if
> > something decides it is not in fact the final frame.
> I can address this by adding an explicit comment to that effect.
> For example, define a separate function to check for the final frame:
> /*
> * Check if this is the final frame. Unwind must stop at the final
> * frame.
> */
> static inline bool unwind_is_final_frame(struct task_struct *task,
> struct stackframe *frame)
> {
> return frame->fp == (unsigned long)task_pt_regs(task)->stackframe;
> }
> Then, use this function in unwind_check_reliability() and unwind_continue().
> Is this acceptable?
Yes, I think that should address the issue - I'd have to see it in
context to be sure but it does make it clear that the same check is
being done which was the main thing.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v11 4/5] arm64: Introduce stack trace reliability checks in the unwinder
2021-11-26 13:29 ` Mark Brown
@ 2021-11-26 17:23 ` Madhavan T. Venkataraman
0 siblings, 0 replies; 21+ messages in thread
From: Madhavan T. Venkataraman @ 2021-11-26 17:23 UTC (permalink / raw)
To: Mark Brown
Cc: mark.rutland, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
On 11/26/21 7:29 AM, Mark Brown wrote:
> On Thu, Nov 25, 2021 at 10:59:27AM -0600, Madhavan T. Venkataraman wrote:
>> On 11/25/21 8:56 AM, Mark Brown wrote:
>>> On Tue, Nov 23, 2021 at 01:37:22PM -0600, madvenka@linux.microsoft.com wrote:
>
>>> Probably also worth noting that this doesn't select
>>> HAVE_RELIABLE_STACKTRACE which is what any actual users are going to use
>>> to identify if the architecture has the feature. I would have been
>>> tempted to add arch_stack_walk() as a separate patch but equally having
>>> the user code there (even if it itself can't yet be used...) helps with
>>> reviewing the actual unwinder so I don't mind.
>
>> I did not select HAVE_RELIABLE_STACKTRACE just in case we think that some
>> more reliability checks need to be added. But if reviewers agree
>> that this patch series contains all the reliability checks we need, I
>> will add a patch to select HAVE_RELIABLE_STACKTRACE to the series.
>
> I agree that more checks probably need to be added, might be worth
> throwing that patch into the end of the series though to provide a place
> to discuss what exactly we need. My main thought here was that it's
> worth explicitly highlighting in this change that the Kconfig bit isn't
> glued up here so reviewers notice that's what's happening.
>
OK. I will add the patch to the next version.
>>>> +static void unwind_check_reliability(struct task_struct *task,
>>>> + struct stackframe *frame)
>>>> +{
>>>> + if (frame->fp == (unsigned long)task_pt_regs(task)->stackframe) {
>>>> + /* Final frame; no more unwind, no need to check reliability */
>>>> + return;
>>>> + }
>
>>> If the unwinder carries on for some reason (the code for that is
>>> elsewhere and may be updated separately...) then this will start
>>> checking again. I'm not sure if this is a *problem* as such but the
>>> thing about this being the final frame coupled with not actually
>>> explicitly stopping the unwind here makes me think this should at least
>>> be clearer, the comment begs the question about what happens if
>>> something decides it is not in fact the final frame.
>
>> I can address this by adding an explicit comment to that effect.
>> For example, define a separate function to check for the final frame:
>
>> /*
>> * Check if this is the final frame. Unwind must stop at the final
>> * frame.
>> */
>> static inline bool unwind_is_final_frame(struct task_struct *task,
>> struct stackframe *frame)
>> {
>> return frame->fp == (unsigned long)task_pt_regs(task)->stackframe;
>> }
>
>> Then, use this function in unwind_check_reliability() and unwind_continue().
>
>> Is this acceptable?
>
> Yes, I think that should address the issue - I'd have to see it in
> context to be sure but it does make it clear that the same check is
> being done which was the main thing.
>
OK. I will make the above changes in the next version.
Thanks.
Madhavan
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v11 5/5] arm64: Create a list of SYM_CODE functions, check return PC against list
2021-11-23 19:37 ` [PATCH v11 0/5] arm64: Reorganize the unwinder and implement stack trace reliability checks madvenka
` (3 preceding siblings ...)
2021-11-23 19:37 ` [PATCH v11 4/5] arm64: Introduce stack trace reliability checks in the unwinder madvenka
@ 2021-11-23 19:37 ` madvenka
2021-11-25 15:05 ` Mark Brown
4 siblings, 1 reply; 21+ messages in thread
From: madvenka @ 2021-11-23 19:37 UTC (permalink / raw)
To: mark.rutland, broonie, jpoimboe, ardb, nobuta.keiya,
sjitindarsingh, catalin.marinas, will, jmorris, linux-arm-kernel,
live-patching, linux-kernel, madvenka
From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
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_is_reliable() that compares a return
PC with sym_code_functions[]. If there is a match, then return failure.
Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
---
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 77eb00e45558..e47f852ad12a 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -18,12 +18,41 @@
#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 task_struct *task,
struct stackframe *frame)
{
+ const struct code_range *range;
+ unsigned long pc;
+ int i;
+
if (frame->fp == (unsigned long)task_pt_regs(task)->stackframe) {
/* Final frame; no more unwind, no need to check reliability */
return;
@@ -36,6 +65,32 @@ static void unwind_check_reliability(struct task_struct *task,
*/
if (!__kernel_text_address(frame->pc))
frame->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 = frame->pc;
+ for (i = 0; i < num_sym_code_functions; i++) {
+ range = &sym_code_functions[i];
+ if (pc >= range->start && pc < range->end) {
+ frame->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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v11 5/5] arm64: Create a list of SYM_CODE functions, check return PC against list
2021-11-23 19:37 ` [PATCH v11 5/5] arm64: Create a list of SYM_CODE functions, check return PC against list madvenka
@ 2021-11-25 15:05 ` Mark Brown
0 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2021-11-25 15:05 UTC (permalink / raw)
To: madvenka
Cc: mark.rutland, jpoimboe, ardb, nobuta.keiya, sjitindarsingh,
catalin.marinas, will, jmorris, linux-arm-kernel, live-patching,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 382 bytes --]
On Tue, Nov 23, 2021 at 01:37:23PM -0600, madvenka@linux.microsoft.com wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>
> 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.
Reviewed-by: Mark Brown <broonie@kernel.org>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread