| Message ID | 20211015025847.17694-5-madvenka@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Reorganize the unwinder and implement stack trace reliability checks | expand |
On Thu, Oct 14, 2021 at 09:58:40PM -0500, madvenka@linux.microsoft.com wrote: > From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > > Currently, return_address() in ARM64 code walks the stack using > start_backtrace() and walk_stackframe(). Make it use arch_stack_walk() > instead. This makes maintenance easier. Reviewed-by: Mark Brown <broonie@kernel.org>
Thanks. On 10/20/21 10:03 AM, Mark Brown wrote: > On Thu, Oct 14, 2021 at 09:58:40PM -0500, madvenka@linux.microsoft.com wrote: >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >> >> Currently, return_address() in ARM64 code walks the stack using >> start_backtrace() and walk_stackframe(). Make it use arch_stack_walk() >> instead. This makes maintenance easier. > > Reviewed-by: Mark Brown <broonie@kernel.org> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thu, Oct 14, 2021 at 09:58:40PM -0500, madvenka@linux.microsoft.com wrote: > From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > > Currently, return_address() in ARM64 code walks the stack using > start_backtrace() and walk_stackframe(). Make it use arch_stack_walk() > instead. This makes maintenance easier. > > Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> > --- > arch/arm64/kernel/return_address.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c > index a6d18755652f..92a0f4d434e4 100644 > --- a/arch/arm64/kernel/return_address.c > +++ b/arch/arm64/kernel/return_address.c > @@ -35,15 +35,11 @@ NOKPROBE_SYMBOL(save_return_addr); > void *return_address(unsigned int level) > { > struct return_address_data data; > - struct stackframe frame; > > data.level = level + 2; > data.addr = NULL; > > - start_backtrace(&frame, > - (unsigned long)__builtin_frame_address(0), > - (unsigned long)return_address); > - walk_stackframe(current, &frame, save_return_addr, &data); > + arch_stack_walk(save_return_addr, &data, current, NULL); This looks equivalent to me. Previously the arguments to start_backtrace() meant that walk_stackframe would report return_address(), then the caller of return_address(), and so on. As arch_stack_walk() starts from its immediate caller (i.e. return_address()), that should result in the same trace. It would be nice if we could note something to that effect in the commit message. I had a play with ftrace, which uses return_address(), and that all looks sound. > > if (!data.level) > return data.addr; The end of this function currently does: if (!data.level) return data.addr; else return NULL; ... but since we initialize data.addr to NULL, and save_return_addr() only writes to data.addr when called at the correct level, we can simplify that to: return data.addr; Regardles of that cleanup: Reviewed-by: Mark Rutland <mark.rutland@arm.com> Tested-by: Mark Rutland <mark.rutland@arm.com> I'll continue reviewing the series next week. Thanks, Mark.
On 10/22/21 1:51 PM, Mark Rutland wrote: > On Thu, Oct 14, 2021 at 09:58:40PM -0500, madvenka@linux.microsoft.com wrote: >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >> >> Currently, return_address() in ARM64 code walks the stack using >> start_backtrace() and walk_stackframe(). Make it use arch_stack_walk() >> instead. This makes maintenance easier. >> >> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> >> --- >> arch/arm64/kernel/return_address.c | 6 +----- >> 1 file changed, 1 insertion(+), 5 deletions(-) >> >> diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c >> index a6d18755652f..92a0f4d434e4 100644 >> --- a/arch/arm64/kernel/return_address.c >> +++ b/arch/arm64/kernel/return_address.c >> @@ -35,15 +35,11 @@ NOKPROBE_SYMBOL(save_return_addr); >> void *return_address(unsigned int level) >> { >> struct return_address_data data; >> - struct stackframe frame; >> >> data.level = level + 2; >> data.addr = NULL; >> >> - start_backtrace(&frame, >> - (unsigned long)__builtin_frame_address(0), >> - (unsigned long)return_address); >> - walk_stackframe(current, &frame, save_return_addr, &data); >> + arch_stack_walk(save_return_addr, &data, current, NULL); > > This looks equivalent to me. Previously the arguments to > start_backtrace() meant that walk_stackframe would report > return_address(), then the caller of return_address(), and so on. As > arch_stack_walk() starts from its immediate caller (i.e. > return_address()), that should result in the same trace. > > It would be nice if we could note something to that effect in the commit > message. > Will do. > I had a play with ftrace, which uses return_address(), and that all > looks sound. > Thanks a lot! >> >> if (!data.level) >> return data.addr; > > The end of this function currently does: > > if (!data.level) > return data.addr; > else > return NULL; > > ... but since we initialize data.addr to NULL, and save_return_addr() > only writes to data.addr when called at the correct level, we can > simplify that to: > > return data.addr; > OK. I will make this change. > Regardles of that cleanup: > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > Tested-by: Mark Rutland <mark.rutland@arm.com> > Thanks a lot! > I'll continue reviewing the series next week. > Great! Madhavan
diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c index a6d18755652f..92a0f4d434e4 100644 --- a/arch/arm64/kernel/return_address.c +++ b/arch/arm64/kernel/return_address.c @@ -35,15 +35,11 @@ NOKPROBE_SYMBOL(save_return_addr); void *return_address(unsigned int level) { struct return_address_data data; - struct stackframe frame; data.level = level + 2; data.addr = NULL; - start_backtrace(&frame, - (unsigned long)__builtin_frame_address(0), - (unsigned long)return_address); - walk_stackframe(current, &frame, save_return_addr, &data); + arch_stack_walk(save_return_addr, &data, current, NULL); if (!data.level) return data.addr;