All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Jones <andrew.jones@linux.dev>
To: 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, npiggin@gmail.com, frankja@linux.ibm.com,
	imbrenda@linux.ibm.com, nrb@linux.ibm.com
Subject: [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace
Date: Wed, 28 Feb 2024 16:04:19 +0100	[thread overview]
Message-ID: <20240228150416.248948-18-andrew.jones@linux.dev> (raw)
In-Reply-To: <20240228150416.248948-15-andrew.jones@linux.dev>

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
diff --git a/lib/x86/stack.c b/lib/x86/stack.c
index 5ecd97ce90b9..58ab6c4b293a 100644
--- a/lib/x86/stack.c
+++ b/lib/x86/stack.c
@@ -1,12 +1,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 = 0;
 	const unsigned long *bp = (unsigned long *) frame;
 
+	if (current_frame)
+		bp = __builtin_frame_address(0);
+
 	if (walking) {
 		printf("RECURSIVE STACK WALK!!!\n");
 		return 0;
@@ -23,9 +27,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
 	walking = 0;
 	return depth;
 }
-
-int backtrace(const void **return_addrs, int max_depth)
-{
-	return backtrace_frame(__builtin_frame_address(0), return_addrs,
-			       max_depth);
-}
-- 
2.43.0


WARNING: multiple messages have this Message-ID (diff)
From: Andrew Jones <andrew.jones@linux.dev>
To: kvm@vger.kernel.org, kvm-riscv@lists.infradead.org
Cc: lvivier@redhat.com, linux-s390@vger.kernel.org, thuth@redhat.com,
	nrb@linux.ibm.com, frankja@linux.ibm.com, npiggin@gmail.com,
	kvmarm@lists.linux.dev, pbonzini@redhat.com,
	imbrenda@linux.ibm.com, linuxppc-dev@lists.ozlabs.org
Subject: [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace
Date: Wed, 28 Feb 2024 16:04:19 +0100	[thread overview]
Message-ID: <20240228150416.248948-18-andrew.jones@linux.dev> (raw)
In-Reply-To: <20240228150416.248948-15-andrew.jones@linux.dev>

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
diff --git a/lib/x86/stack.c b/lib/x86/stack.c
index 5ecd97ce90b9..58ab6c4b293a 100644
--- a/lib/x86/stack.c
+++ b/lib/x86/stack.c
@@ -1,12 +1,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 = 0;
 	const unsigned long *bp = (unsigned long *) frame;
 
+	if (current_frame)
+		bp = __builtin_frame_address(0);
+
 	if (walking) {
 		printf("RECURSIVE STACK WALK!!!\n");
 		return 0;
@@ -23,9 +27,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
 	walking = 0;
 	return depth;
 }
-
-int backtrace(const void **return_addrs, int max_depth)
-{
-	return backtrace_frame(__builtin_frame_address(0), return_addrs,
-			       max_depth);
-}
-- 
2.43.0


  parent reply	other threads:[~2024-02-28 15:04 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-28 15:04 [kvm-unit-tests PATCH 00/13] Enable EFI support Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 01/13] riscv: Call abort instead of assert on unhandled exceptions Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 02/13] riscv: show_regs: Prepare for EFI images Andrew Jones
2024-02-28 15:04 ` Andrew Jones [this message]
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-28 17:33     ` Claudio Imbrenda
2024-02-29  3:31   ` Nicholas Piggin
2024-02-29  3:31     ` Nicholas Piggin
2024-02-29 11:48     ` Andrew Jones
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-28 15:04   ` Andrew Jones
2024-02-29  3:49   ` Nicholas Piggin
2024-02-29  3:49     ` Nicholas Piggin
2024-02-29 11:54     ` Andrew Jones
2024-02-29 11:54       ` Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 05/13] riscv: Import gnu-efi files Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 06/13] riscv: Tweak the gnu-efi imported code Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 07/13] riscv: Enable building for EFI Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 08/13] riscv: efi: Switch stack in _start Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 09/13] efi: Add support for obtaining the boot hartid Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 10/13] riscv: Refactor setup code Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 11/13] riscv: Enable EFI boot Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 12/13] riscv: efi: Add run script Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 13/13] riscv: efi: Use efi-direct by default 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=20240228150416.248948-18-andrew.jones@linux.dev \
    --to=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=npiggin@gmail.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.