[RFC PATCH v1 06/16] unwind_user: Enable archs that define CFA = SP_callsite + offset

Jens Remus posted 16 patches 2 months, 4 weeks ago
[RFC PATCH v1 06/16] unwind_user: Enable archs that define CFA = SP_callsite + offset
Posted by Jens Remus 2 months, 4 weeks ago
Most architectures define their CFA as the value of the stack pointer
(SP) at the call site in the previous frame, as suggested by the DWARF
standard:

  CFA = <SP at call site>

Enable unwinding of user space for architectures, such as s390, which
define their CFA as the value of the SP at the call site in the previous
frame with an offset:

  CFA = <SP at call site> + offset

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Alternatively the conditional definition of generic_sframe_sp_val_off()
    could be moved into kernel/unwind/sframe.c.

 arch/x86/include/asm/unwind_user.h       |  2 ++
 include/asm-generic/Kbuild               |  1 +
 include/asm-generic/unwind_user_sframe.h | 30 ++++++++++++++++++++++++
 include/linux/unwind_user_types.h        |  1 +
 kernel/unwind/sframe.c                   |  2 ++
 kernel/unwind/user.c                     |  8 ++++---
 6 files changed, 41 insertions(+), 3 deletions(-)
 create mode 100644 include/asm-generic/unwind_user_sframe.h

diff --git a/arch/x86/include/asm/unwind_user.h b/arch/x86/include/asm/unwind_user.h
index 19634a73612d..c2881840adf4 100644
--- a/arch/x86/include/asm/unwind_user.h
+++ b/arch/x86/include/asm/unwind_user.h
@@ -8,6 +8,7 @@
 	.cfa_off	= (s32)sizeof(long) *  2,				\
 	.ra_off		= (s32)sizeof(long) * -1,				\
 	.fp_off		= (s32)sizeof(long) * -2,				\
+	.sp_val_off	= (s32)0,						\
 	.use_fp		= true,
 
 #ifdef CONFIG_IA32_EMULATION
@@ -16,6 +17,7 @@
 	.cfa_off	= (s32)sizeof(u32)  *  2,				\
 	.ra_off		= (s32)sizeof(u32)  * -1,				\
 	.fp_off		= (s32)sizeof(u32)  * -2,				\
+	.sp_val_off	= (s32)0,						\
 	.use_fp		= true,
 
 #define in_compat_mode(regs) !user_64bit_mode(regs)
diff --git a/include/asm-generic/Kbuild b/include/asm-generic/Kbuild
index b797a2434396..64a15cde776c 100644
--- a/include/asm-generic/Kbuild
+++ b/include/asm-generic/Kbuild
@@ -60,6 +60,7 @@ mandatory-y += topology.h
 mandatory-y += trace_clock.h
 mandatory-y += uaccess.h
 mandatory-y += unwind_user.h
+mandatory-y += unwind_user_sframe.h
 mandatory-y += unwind_user_types.h
 mandatory-y += vermagic.h
 mandatory-y += vga.h
diff --git a/include/asm-generic/unwind_user_sframe.h b/include/asm-generic/unwind_user_sframe.h
new file mode 100644
index 000000000000..6c87a7f29861
--- /dev/null
+++ b/include/asm-generic/unwind_user_sframe.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_UNWIND_USER_SFRAME_H
+#define _ASM_GENERIC_UNWIND_USER_SFRAME_H
+
+#include <linux/types.h>
+
+/**
+ * generic_sframe_sp_val_off - Get generic SP value offset from CFA.
+ *
+ * During unwinding the stack pointer (SP) at call site can be derived
+ * from the Canonical Frame Address (CFA) as follows:
+ *
+ *   SP = CFA + SP_val_off
+ *
+ * Most architectures define the CFA as value of SP at call site, as
+ * suggested by DWARF. For those architectures the SP value offset
+ * from CFA is 0, so that the SP at call site computes as:
+ *
+ *   SP = CFA
+ *
+ * Returns the generic SP value offset from CFA of 0.
+ */
+static inline s32 generic_sframe_sp_val_off(void)
+{
+	return 0;
+}
+
+#define sframe_sp_val_off generic_sframe_sp_val_off
+
+#endif /* _ASM_GENERIC_UNWIND_USER_SFRAME_H */
diff --git a/include/linux/unwind_user_types.h b/include/linux/unwind_user_types.h
index 4d50476e950e..8050a3237a03 100644
--- a/include/linux/unwind_user_types.h
+++ b/include/linux/unwind_user_types.h
@@ -25,6 +25,7 @@ struct unwind_user_frame {
 	s32 cfa_off;
 	s32 ra_off;
 	s32 fp_off;
+	s32 sp_val_off;
 	bool use_fp;
 };
 
diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
index 6159f072bdb6..acbf791e713b 100644
--- a/kernel/unwind/sframe.c
+++ b/kernel/unwind/sframe.c
@@ -12,6 +12,7 @@
 #include <linux/mm.h>
 #include <linux/string_helpers.h>
 #include <linux/sframe.h>
+#include <asm/unwind_user_sframe.h>
 #include <linux/unwind_user_types.h>
 
 #include "sframe.h"
@@ -313,6 +314,7 @@ static __always_inline int __find_fre(struct sframe_section *sec,
 	frame->ra_off  = fre->ra_off;
 	frame->fp_off  = fre->fp_off;
 	frame->use_fp  = SFRAME_FRE_CFA_BASE_REG_ID(fre->info) == SFRAME_BASE_REG_FP;
+	frame->sp_val_off = sframe_sp_val_off();
 
 	return 0;
 }
diff --git a/kernel/unwind/user.c b/kernel/unwind/user.c
index d0181c636c6b..45c8c6932ba6 100644
--- a/kernel/unwind/user.c
+++ b/kernel/unwind/user.c
@@ -52,7 +52,7 @@ static int unwind_user_next(struct unwind_user_state *state)
 {
 	struct unwind_user_frame *frame;
 	struct unwind_user_frame _frame;
-	unsigned long cfa = 0, fp, ra = 0;
+	unsigned long cfa = 0, sp, fp, ra = 0;
 	unsigned int shift;
 
 	if (state->done)
@@ -84,8 +84,10 @@ static int unwind_user_next(struct unwind_user_state *state)
 	}
 	cfa += frame->cfa_off;
 
+	/* Get the Stack Pointer (SP) */
+	sp = cfa + frame->sp_val_off;
 	/* Make sure that stack is not going in wrong direction */
-	if (cfa <= state->sp)
+	if (sp <= state->sp)
 		goto done;
 
 	/* Make sure that the address is word aligned */
@@ -102,7 +104,7 @@ static int unwind_user_next(struct unwind_user_state *state)
 		goto done;
 
 	state->ip = ra;
-	state->sp = cfa;
+	state->sp = sp;
 	if (frame->fp_off)
 		state->fp = fp;
 
-- 
2.48.1
Re: [RFC PATCH v1 06/16] unwind_user: Enable archs that define CFA = SP_callsite + offset
Posted by Josh Poimboeuf 2 months, 3 weeks ago
On Thu, Jul 10, 2025 at 06:35:12PM +0200, Jens Remus wrote:
> Most architectures define their CFA as the value of the stack pointer
> (SP) at the call site in the previous frame, as suggested by the DWARF
> standard:
> 
>   CFA = <SP at call site>
> 
> Enable unwinding of user space for architectures, such as s390, which
> define their CFA as the value of the SP at the call site in the previous
> frame with an offset:
> 
>   CFA = <SP at call site> + offset

This is a bit confusing, as the comment and code define it as

    SP = CFA + offset

Should the commit log be updated to match that?

> +++ b/arch/x86/include/asm/unwind_user.h
> @@ -8,6 +8,7 @@
>  	.cfa_off	= (s32)sizeof(long) *  2,				\
>  	.ra_off		= (s32)sizeof(long) * -1,				\
>  	.fp_off		= (s32)sizeof(long) * -2,				\
> +	.sp_val_off	= (s32)0,						\

IIUC, this is similar to ra_off and fp_off in that its an offset from
the CFA.  Can we call it "sp_off"?

-- 
Josh
Re: [RFC PATCH v1 06/16] unwind_user: Enable archs that define CFA = SP_callsite + offset
Posted by Jens Remus 2 months, 3 weeks ago
On 16.07.2025 23:32, Josh Poimboeuf wrote:
> On Thu, Jul 10, 2025 at 06:35:12PM +0200, Jens Remus wrote:
>> Most architectures define their CFA as the value of the stack pointer
>> (SP) at the call site in the previous frame, as suggested by the DWARF
>> standard:
>>
>>   CFA = <SP at call site>
>>
>> Enable unwinding of user space for architectures, such as s390, which
>> define their CFA as the value of the SP at the call site in the previous
>> frame with an offset:
>>
>>   CFA = <SP at call site> + offset
> 
> This is a bit confusing, as the comment and code define it as
> 
>     SP = CFA + offset
> 
> Should the commit log be updated to match that?

I agree that the commit message is confusing. Would it help if I replace
it with the following:

Most architectures define their CFA as the value of the stack pointer
(SP) at the call site in the previous frame, as suggested by the DWARF
standard.  Therefore the SP at call site can be unwound using an
implicitly assumed value offset from CFA rule with an offset of zero:

  .cfi_val_offset <SP>, 0

As a result the SP at call site computes as follows:

  SP = CFA

Enable unwinding of user space for architectures, such as s390, which
define their CFA as the value of the SP at the call site in the previous
frame with an offset.  Do so by enabling architectures to override the
default SP value offset from CFA of zero with an architecture-specific
one:

  .cfi_val_offset <SP>, offset
  
So that the SP at call site computes as follows:

  SP = CFA + offset

>> +++ b/arch/x86/include/asm/unwind_user.h
>> @@ -8,6 +8,7 @@
>>  	.cfa_off	= (s32)sizeof(long) *  2,				\
>>  	.ra_off		= (s32)sizeof(long) * -1,				\
>>  	.fp_off		= (s32)sizeof(long) * -2,				\
>> +	.sp_val_off	= (s32)0,						\
> 
> IIUC, this is similar to ra_off and fp_off in that its an offset from
> the CFA.  Can we call it "sp_off"?

My intent was to use the terminology from DWARF CFI (i.e. "offset(N)"
and "val_offset(N)") and the related assembler CFI directives:

  .cfi_offset register, offset:  Previous value of register is saved at
                                 offset from CFA.

  .cfi_val_offset register, offset:  Previous value of register is
                                     CFA + offset. 

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com

IBM

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/

Re: [RFC PATCH v1 06/16] unwind_user: Enable archs that define CFA = SP_callsite + offset
Posted by Josh Poimboeuf 2 months, 3 weeks ago
On Thu, Jul 17, 2025 at 11:27:45AM +0200, Jens Remus wrote:
> On 16.07.2025 23:32, Josh Poimboeuf wrote:
> > On Thu, Jul 10, 2025 at 06:35:12PM +0200, Jens Remus wrote:
> >> Most architectures define their CFA as the value of the stack pointer
> >> (SP) at the call site in the previous frame, as suggested by the DWARF
> >> standard:
> >>
> >>   CFA = <SP at call site>
> >>
> >> Enable unwinding of user space for architectures, such as s390, which
> >> define their CFA as the value of the SP at the call site in the previous
> >> frame with an offset:
> >>
> >>   CFA = <SP at call site> + offset
> > 
> > This is a bit confusing, as the comment and code define it as
> > 
> >     SP = CFA + offset
> > 
> > Should the commit log be updated to match that?
> 
> I agree that the commit message is confusing. Would it help if I replace
> it with the following:
> 
> Most architectures define their CFA as the value of the stack pointer
> (SP) at the call site in the previous frame, as suggested by the DWARF
> standard.  Therefore the SP at call site can be unwound using an
> implicitly assumed value offset from CFA rule with an offset of zero:
> 
>   .cfi_val_offset <SP>, 0
> 
> As a result the SP at call site computes as follows:
> 
>   SP = CFA
> 
> Enable unwinding of user space for architectures, such as s390, which
> define their CFA as the value of the SP at the call site in the previous
> frame with an offset.  Do so by enabling architectures to override the
> default SP value offset from CFA of zero with an architecture-specific
> one:
> 
>   .cfi_val_offset <SP>, offset
>   
> So that the SP at call site computes as follows:
> 
>   SP = CFA + offset

Looks good to me, thanks!

> >> +++ b/arch/x86/include/asm/unwind_user.h
> >> @@ -8,6 +8,7 @@
> >>  	.cfa_off	= (s32)sizeof(long) *  2,				\
> >>  	.ra_off		= (s32)sizeof(long) * -1,				\
> >>  	.fp_off		= (s32)sizeof(long) * -2,				\
> >> +	.sp_val_off	= (s32)0,						\
> > 
> > IIUC, this is similar to ra_off and fp_off in that its an offset from
> > the CFA.  Can we call it "sp_off"?
> 
> My intent was to use the terminology from DWARF CFI (i.e. "offset(N)"
> and "val_offset(N)") and the related assembler CFI directives:
> 
>   .cfi_offset register, offset:  Previous value of register is saved at
>                                  offset from CFA.
> 
>   .cfi_val_offset register, offset:  Previous value of register is
>                                      CFA + offset. 

The distinction between "cfi_offset" and "cfi_val_offset" is confusing,
unless one already happens to know CFI syntax (not likely for us kernel
developers).

We don't need to match the DWARF CFI directive naming.  Let's instead
optimize for readability.

I think "sp_off" is fine here, its semantics are similar to the existing
cfa_off field.

The semantics of ra_off and fp_off are different, but those are getting
removed in favor of nested structs in a later patch anyway.

-- 
Josh