From: "Nicholas Piggin" <npiggin@gmail.com>
To: "Andrew Jones" <andrew.jones@linux.dev>, <kvm@vger.kernel.org>,
<kvm-riscv@lists.infradead.org>
Cc: <pbonzini@redhat.com>, <thuth@redhat.com>,
<kvmarm@lists.linux.dev>, <linuxppc-dev@lists.ozlabs.org>,
<linux-s390@vger.kernel.org>, <lvivier@redhat.com>,
<frankja@linux.ibm.com>, <imbrenda@linux.ibm.com>,
<nrb@linux.ibm.com>
Subject: Re: [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace
Date: Thu, 29 Feb 2024 13:31:52 +1000 [thread overview]
Message-ID: <CZH8V1T0Z3QN.1ZVFAXR4B96BZ@wheely> (raw)
In-Reply-To: <20240228150416.248948-18-andrew.jones@linux.dev>
On Thu Feb 29, 2024 at 1:04 AM AEST, Andrew Jones wrote:
> We should never pass the result of __builtin_frame_address(0) to
> another function since the compiler is within its rights to pop the
> frame to which it points before making the function call, as may be
> done for tail calls. Nobody has complained about backtrace(), so
> likely all compilations have been inlining backtrace_frame(), not
> dropping the frame on the tail call, or nobody is looking at traces.
> However, for riscv, when built for EFI, it does drop the frame on the
> tail call, and it was noticed. Preemptively fix backtrace() for all
> architectures.
>
> Fixes: 52266791750d ("lib: backtrace printing")
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
> lib/arm/stack.c | 13 +++++--------
> lib/arm64/stack.c | 12 +++++-------
> lib/riscv/stack.c | 12 +++++-------
> lib/s390x/stack.c | 12 +++++-------
> lib/stack.h | 24 +++++++++++++++++-------
> lib/x86/stack.c | 12 +++++-------
> 6 files changed, 42 insertions(+), 43 deletions(-)
>
> diff --git a/lib/arm/stack.c b/lib/arm/stack.c
> index 7d081be7c6d0..66d18b47ea53 100644
> --- a/lib/arm/stack.c
> +++ b/lib/arm/stack.c
> @@ -8,13 +8,16 @@
> #include <libcflat.h>
> #include <stack.h>
>
> -int backtrace_frame(const void *frame, const void **return_addrs,
> - int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> + int max_depth, bool current_frame)
> {
> static int walking;
> int depth;
> const unsigned long *fp = (unsigned long *)frame;
>
> + if (current_frame)
> + fp = __builtin_frame_address(0);
> +
> if (walking) {
> printf("RECURSIVE STACK WALK!!!\n");
> return 0;
> @@ -33,9 +36,3 @@ int backtrace_frame(const void *frame, const void **return_addrs,
> walking = 0;
> return depth;
> }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> - return backtrace_frame(__builtin_frame_address(0),
> - return_addrs, max_depth);
> -}
> diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c
> index 82611f4b1815..f5eb57fd8892 100644
> --- a/lib/arm64/stack.c
> +++ b/lib/arm64/stack.c
> @@ -8,7 +8,8 @@
>
> extern char vector_stub_start, vector_stub_end;
>
> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> + int max_depth, bool current_frame)
> {
> const void *fp = frame;
> static bool walking;
> @@ -17,6 +18,9 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> bool is_exception = false;
> unsigned long addr;
>
> + if (current_frame)
> + fp = __builtin_frame_address(0);
> +
> if (walking) {
> printf("RECURSIVE STACK WALK!!!\n");
> return 0;
> @@ -54,9 +58,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> walking = false;
> return depth;
> }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> - return backtrace_frame(__builtin_frame_address(0),
> - return_addrs, max_depth);
> -}
> diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c
> index 712a5478d547..d865594b9671 100644
> --- a/lib/riscv/stack.c
> +++ b/lib/riscv/stack.c
> @@ -2,12 +2,16 @@
> #include <libcflat.h>
> #include <stack.h>
>
> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> + int max_depth, bool current_frame)
> {
> static bool walking;
> const unsigned long *fp = (unsigned long *)frame;
> int depth;
>
> + if (current_frame)
> + fp = __builtin_frame_address(0);
> +
> if (walking) {
> printf("RECURSIVE STACK WALK!!!\n");
> return 0;
> @@ -24,9 +28,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> walking = false;
> return depth;
> }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> - return backtrace_frame(__builtin_frame_address(0),
> - return_addrs, max_depth);
> -}
> diff --git a/lib/s390x/stack.c b/lib/s390x/stack.c
> index 9f234a12adf6..d194f654e94d 100644
> --- a/lib/s390x/stack.c
> +++ b/lib/s390x/stack.c
> @@ -14,11 +14,15 @@
> #include <stack.h>
> #include <asm/arch_def.h>
>
> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
> +int arch_backtrace_frame(const void *frame, const void **return_addrs,
> + int max_depth, bool current_frame)
> {
> int depth = 0;
> struct stack_frame *stack = (struct stack_frame *)frame;
>
> + if (current_frame)
> + stack = __builtin_frame_address(0);
> +
> for (depth = 0; stack && depth < max_depth; depth++) {
> return_addrs[depth] = (void *)stack->grs[8];
> stack = stack->back_chain;
> @@ -28,9 +32,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
>
> return depth;
> }
> -
> -int backtrace(const void **return_addrs, int max_depth)
> -{
> - return backtrace_frame(__builtin_frame_address(0),
> - return_addrs, max_depth);
> -}
> diff --git a/lib/stack.h b/lib/stack.h
> index 10fc2f793354..6edc84344b51 100644
> --- a/lib/stack.h
> +++ b/lib/stack.h
> @@ -11,17 +11,27 @@
> #include <asm/stack.h>
>
> #ifdef HAVE_ARCH_BACKTRACE_FRAME
> -extern int backtrace_frame(const void *frame, const void **return_addrs,
> - int max_depth);
> +extern int arch_backtrace_frame(const void *frame, const void **return_addrs,
> + int max_depth, bool current_frame);
> +
> +static inline int backtrace_frame(const void *frame, const void **return_addrs,
> + int max_depth)
> +{
> + return arch_backtrace_frame(frame, return_addrs, max_depth, false);
> +}
> +
> +static inline int backtrace(const void **return_addrs, int max_depth)
> +{
> + return arch_backtrace_frame(NULL, return_addrs, max_depth, true);
> +}
> #else
> -static inline int
> -backtrace_frame(const void *frame __unused, const void **return_addrs __unused,
> - int max_depth __unused)
> +extern int backtrace(const void **return_addrs, int max_depth);
> +
> +static inline int backtrace_frame(const void *frame, const void **return_addrs,
> + int max_depth)
> {
> return 0;
> }
> #endif
>
> -extern int backtrace(const void **return_addrs, int max_depth);
> -
> #endif
Is there a reason to add the inline wrappers rather than just externs
and drop the arch_ prefix?
Do we want to just generally have all arch specific functions have an
arch_ prefix? Fine by me.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
I'm fine to rebase the powerpc patch on top of this if it goes in first.
Thanks for the heads up.
Thanks,
Nick
next prev parent reply other threads:[~2024-02-29 3:32 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240228150416.248948-15-andrew.jones@linux.dev>
2024-02-28 15:04 ` [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace Andrew Jones
2024-02-28 17:33 ` Claudio Imbrenda
2024-02-29 3:31 ` Nicholas Piggin [this message]
2024-02-29 11:48 ` Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 04/13] treewide: lib/stack: Make base_address arch specific Andrew Jones
2024-02-29 3:49 ` Nicholas Piggin
2024-02-29 11:54 ` Andrew Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CZH8V1T0Z3QN.1ZVFAXR4B96BZ@wheely \
--to=npiggin@gmail.com \
--cc=andrew.jones@linux.dev \
--cc=frankja@linux.ibm.com \
--cc=imbrenda@linux.ibm.com \
--cc=kvm-riscv@lists.infradead.org \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.linux.dev \
--cc=linux-s390@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lvivier@redhat.com \
--cc=nrb@linux.ibm.com \
--cc=pbonzini@redhat.com \
--cc=thuth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox