| Message ID | 20210513022709.983982-2-pcc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/3] arm64: Change the on_*stack functions to take a size argument | expand |
Hi Peter, On Wed, May 12, 2021 at 07:27:08PM -0700, Peter Collingbourne wrote: > The AAPCS places no requirements on the alignment of the frame > record. In theory it could be placed anywhere, although it seems > sensible to require it to be aligned to 8 bytes. With an upcoming > enhancement to tag-based KASAN Clang will begin creating frame records > located at an address that is only aligned to 8 bytes. Accommodate > such frame records in the stack unwinding code. > > As pointed out by Mark Rutland, the userspace stack unwinding code > has the same problem, so fix it there as well. > > Signed-off-by: Peter Collingbourne <pcc@google.com> > Link: https://linux-review.googlesource.com/id/Ia22c375230e67ca055e9e4bb639383567f7ad268 > Acked-by: Andrey Konovalov <andreyknvl@gmail.com> > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > Tested-by: Mark Rutland <mark.rutland@arm.com> > --- > v4: > - rebase to 5.13rc1 I think the rebase went slightly wrong; note below. > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index 2fecbf152e80..020e575e5cd3 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -68,7 +68,11 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) > unsigned long fp = frame->fp; > struct stack_info info; > > - if (fp & 0xf) > + /* Terminal record; nothing to unwind */ > + if (!fp) > + return -ENOENT; > + > + if (fp & 0x7) > return -EINVAL; The terminal record check got moved later in the function by commit: 8533d5bfad41e74b ("arm64: stacktrace: restore terminal records") ... so this patch shouldn't re-add it. With that gone (and this just changing the alignemnt check), this looks good to me. Thanks, Mark.
On Thu, May 13, 2021 at 2:16 AM Mark Rutland <mark.rutland@arm.com> wrote: > > Hi Peter, > > On Wed, May 12, 2021 at 07:27:08PM -0700, Peter Collingbourne wrote: > > The AAPCS places no requirements on the alignment of the frame > > record. In theory it could be placed anywhere, although it seems > > sensible to require it to be aligned to 8 bytes. With an upcoming > > enhancement to tag-based KASAN Clang will begin creating frame records > > located at an address that is only aligned to 8 bytes. Accommodate > > such frame records in the stack unwinding code. > > > > As pointed out by Mark Rutland, the userspace stack unwinding code > > has the same problem, so fix it there as well. > > > > Signed-off-by: Peter Collingbourne <pcc@google.com> > > Link: https://linux-review.googlesource.com/id/Ia22c375230e67ca055e9e4bb639383567f7ad268 > > Acked-by: Andrey Konovalov <andreyknvl@gmail.com> > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > > Tested-by: Mark Rutland <mark.rutland@arm.com> > > --- > > v4: > > - rebase to 5.13rc1 > > I think the rebase went slightly wrong; note below. > > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > > index 2fecbf152e80..020e575e5cd3 100644 > > --- a/arch/arm64/kernel/stacktrace.c > > +++ b/arch/arm64/kernel/stacktrace.c > > @@ -68,7 +68,11 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) > > unsigned long fp = frame->fp; > > struct stack_info info; > > > > - if (fp & 0xf) > > + /* Terminal record; nothing to unwind */ > > + if (!fp) > > + return -ENOENT; > > + > > + if (fp & 0x7) > > return -EINVAL; > > The terminal record check got moved later in the function by commit: > > 8533d5bfad41e74b ("arm64: stacktrace: restore terminal records") > > ... so this patch shouldn't re-add it. With that gone (and this just > changing the alignemnt check), this looks good to me. Thanks, fixed in v5. Peter
diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c index 88ff471b0bce..4a72c2727309 100644 --- a/arch/arm64/kernel/perf_callchain.c +++ b/arch/arm64/kernel/perf_callchain.c @@ -116,7 +116,7 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry, tail = (struct frame_tail __user *)regs->regs[29]; while (entry->nr < entry->max_stack && - tail && !((unsigned long)tail & 0xf)) + tail && !((unsigned long)tail & 0x7)) tail = user_backtrace(tail, entry); } else { #ifdef CONFIG_COMPAT diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index 2fecbf152e80..020e575e5cd3 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -68,7 +68,11 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) unsigned long fp = frame->fp; struct stack_info info; - if (fp & 0xf) + /* Terminal record; nothing to unwind */ + if (!fp) + return -ENOENT; + + if (fp & 0x7) return -EINVAL; if (!tsk)