Enable unwinding of user space for architectures, such as s390, that
save the return address (RA) and/or frame pointer (FP) in other
registers.  This is only valid in the topmost frame, for instance when
in a leaf function.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
 arch/Kconfig                             |  7 ++++
 arch/x86/include/asm/unwind_user.h       | 24 +++++++++---
 include/asm-generic/unwind_user.h        | 20 ++++++++++
 include/asm-generic/unwind_user_sframe.h | 24 ++++++++++++
 include/linux/unwind_user_types.h        | 18 ++++++++-
 kernel/unwind/sframe.c                   |  4 +-
 kernel/unwind/user.c                     | 47 ++++++++++++++++++++----
 7 files changed, 126 insertions(+), 18 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 367eaf7e62e0..9e28dffe42cb 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -455,6 +455,13 @@ config HAVE_USER_RA_REG
 	help
 	  The arch passes the return address (RA) in user space in a register.
 
+config HAVE_UNWIND_USER_LOC_REG
+	bool
+	help
+	  The arch potentially saves the return address (RA) and/or frame
+	  pointer (FP) register values in user space in other registers, when
+	  in the topmost frame (e.g. in leaf function).
+
 config SFRAME_VALIDATION
 	bool "Enable .sframe section debugging"
 	depends on HAVE_UNWIND_USER_SFRAME
diff --git a/arch/x86/include/asm/unwind_user.h b/arch/x86/include/asm/unwind_user.h
index c2881840adf4..925d208aa39d 100644
--- a/arch/x86/include/asm/unwind_user.h
+++ b/arch/x86/include/asm/unwind_user.h
@@ -5,18 +5,30 @@
 #include <linux/unwind_user_types.h>
 
 #define ARCH_INIT_USER_FP_FRAME							\
-	.cfa_off	= (s32)sizeof(long) *  2,				\
-	.ra_off		= (s32)sizeof(long) * -1,				\
-	.fp_off		= (s32)sizeof(long) * -2,				\
+	.cfa_off	= (s32)sizeof(long) * 2,				\
+	.ra		= {							\
+		.loc = UNWIND_USER_LOC_STACK,					\
+		.frame_off = (s32)sizeof(long) * -1,				\
+	},									\
+	.fp		= {							\
+		.loc = UNWIND_USER_LOC_STACK,					\
+		.frame_off = (s32)sizeof(long) * -2,				\
+	},									\
 	.sp_val_off	= (s32)0,						\
 	.use_fp		= true,
 
 #ifdef CONFIG_IA32_EMULATION
 
 #define ARCH_INIT_USER_COMPAT_FP_FRAME						\
-	.cfa_off	= (s32)sizeof(u32)  *  2,				\
-	.ra_off		= (s32)sizeof(u32)  * -1,				\
-	.fp_off		= (s32)sizeof(u32)  * -2,				\
+	.cfa_off	= (s32)sizeof(u32) * 2,					\
+	.ra		= {							\
+		.loc = UNWIND_USER_LOC_STACK,					\
+		.frame_off = (s32)sizeof(u32) * -1,				\
+	},									\
+	.fp		= {							\
+		.loc = UNWIND_USER_LOC_STACK,					\
+		.frame_off = (s32)sizeof(u32) * -2,				\
+	},									\
 	.sp_val_off	= (s32)0,						\
 	.use_fp		= true,
 
diff --git a/include/asm-generic/unwind_user.h b/include/asm-generic/unwind_user.h
index b8882b909944..3891b7cfe3b8 100644
--- a/include/asm-generic/unwind_user.h
+++ b/include/asm-generic/unwind_user.h
@@ -2,4 +2,24 @@
 #ifndef _ASM_GENERIC_UNWIND_USER_H
 #define _ASM_GENERIC_UNWIND_USER_H
 
+#include <asm/unwind_user_types.h>
+
+#ifndef unwind_user_get_reg
+
+/**
+ * generic_unwind_user_get_reg - Get register value.
+ * @val: Register value.
+ * @regnum: DWARF register number to obtain the value from.
+ *
+ * Returns zero if successful. Otherwise -EINVAL.
+ */
+static inline int generic_unwind_user_get_reg(unsigned long *val, int regnum)
+{
+	return -EINVAL;
+}
+
+#define unwind_user_get_reg generic_unwind_user_get_reg
+
+#endif /* !unwind_user_get_reg */
+
 #endif /* _ASM_GENERIC_UNWIND_USER_H */
diff --git a/include/asm-generic/unwind_user_sframe.h b/include/asm-generic/unwind_user_sframe.h
index 6c87a7f29861..8cef3e0857b6 100644
--- a/include/asm-generic/unwind_user_sframe.h
+++ b/include/asm-generic/unwind_user_sframe.h
@@ -2,8 +2,31 @@
 #ifndef _ASM_GENERIC_UNWIND_USER_SFRAME_H
 #define _ASM_GENERIC_UNWIND_USER_SFRAME_H
 
+#include <linux/unwind_user_types.h>
 #include <linux/types.h>
 
+/**
+ * generic_sframe_set_frame_reginfo - Populate info to unwind FP/RA register
+ * from SFrame offset.
+ * @reginfo: Unwind info for FP/RA register.
+ * @offset: SFrame offset value.
+ *
+ * A non-zero offset value denotes a stack offset from CFA and indicates
+ * that the register is saved on the stack. A zero offset value indicates
+ * that the register is not saved.
+ */
+static inline void generic_sframe_set_frame_reginfo(
+	struct unwind_user_reginfo *reginfo,
+	s32 offset)
+{
+	if (offset) {
+		reginfo->loc = UNWIND_USER_LOC_STACK;
+		reginfo->frame_off = offset;
+	} else {
+		reginfo->loc = UNWIND_USER_LOC_NONE;
+	}
+}
+
 /**
  * generic_sframe_sp_val_off - Get generic SP value offset from CFA.
  *
@@ -25,6 +48,7 @@ static inline s32 generic_sframe_sp_val_off(void)
 	return 0;
 }
 
+#define sframe_set_frame_reginfo generic_sframe_set_frame_reginfo
 #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 adef01698bb3..57fd16e314cf 100644
--- a/include/linux/unwind_user_types.h
+++ b/include/linux/unwind_user_types.h
@@ -21,10 +21,24 @@ struct unwind_stacktrace {
 	unsigned long	*entries;
 };
 
+enum unwind_user_loc {
+	UNWIND_USER_LOC_NONE,
+	UNWIND_USER_LOC_STACK,
+	UNWIND_USER_LOC_REG,
+};
+
+struct unwind_user_reginfo {
+	enum unwind_user_loc loc;
+	union {
+		s32 frame_off;
+		int regnum;
+	};
+};
+
 struct unwind_user_frame {
 	s32 cfa_off;
-	s32 ra_off;
-	s32 fp_off;
+	struct unwind_user_reginfo ra;
+	struct unwind_user_reginfo fp;
 	s32 sp_val_off;
 	bool use_fp;
 };
diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
index 5bfaf06e6cd2..43ef3a8c4c26 100644
--- a/kernel/unwind/sframe.c
+++ b/kernel/unwind/sframe.c
@@ -314,8 +314,8 @@ static __always_inline int __find_fre(struct sframe_section *sec,
 	}
 
 	frame->cfa_off = fre->cfa_off;
-	frame->ra_off  = fre->ra_off;
-	frame->fp_off  = fre->fp_off;
+	sframe_set_frame_reginfo(&frame->ra, fre->ra_off);
+	sframe_set_frame_reginfo(&frame->fp, 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();
 
diff --git a/kernel/unwind/user.c b/kernel/unwind/user.c
index 03a6da36192f..ee00d39d2a8e 100644
--- a/kernel/unwind/user.c
+++ b/kernel/unwind/user.c
@@ -98,26 +98,57 @@ static int unwind_user_next(struct unwind_user_state *state)
 
 
 	/* Get the Return Address (RA) */
-	if (frame->ra_off) {
+	switch (frame->ra.loc) {
+	case UNWIND_USER_LOC_NONE:
+		if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
+			goto done;
+		ra = user_return_address(task_pt_regs(current));
+		break;
+	case UNWIND_USER_LOC_STACK:
+		if (!frame->ra.frame_off)
+			goto done;
 		/* Make sure that the address is word aligned */
 		shift = sizeof(long) == 4 || compat_fp_state(state) ? 2 : 3;
-		if ((cfa + frame->ra_off) & ((1 << shift) - 1))
+		if ((cfa + frame->ra.frame_off) & ((1 << shift) - 1))
 			goto done;
-		if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
+		if (unwind_get_user_long(ra, cfa + frame->ra.frame_off, state))
 			goto done;
-	} else {
-		if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
+		break;
+	case UNWIND_USER_LOC_REG:
+		if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
 			goto done;
-		ra = user_return_address(task_pt_regs(current));
+		if (unwind_user_get_reg(&ra, frame->ra.regnum))
+			goto done;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		goto done;
 	}
 
 	/* Get the Frame Pointer (FP) */
-	if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, state))
+	switch (frame->fp.loc) {
+	case UNWIND_USER_LOC_NONE:
+		break;
+	case UNWIND_USER_LOC_STACK:
+		if (!frame->fp.frame_off)
+			goto done;
+		if (unwind_get_user_long(fp, cfa + frame->fp.frame_off, state))
+			goto done;
+		break;
+	case UNWIND_USER_LOC_REG:
+		if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
+			goto done;
+		if (unwind_user_get_reg(&fp, frame->fp.regnum))
+			goto done;
+		break;
+	default:
+		WARN_ON_ONCE(1);
 		goto done;
+	}
 
 	state->ip = ra;
 	state->sp = sp;
-	if (frame->fp_off)
+	if (frame->fp.loc != UNWIND_USER_LOC_NONE)
 		state->fp = fp;
 
 	arch_unwind_user_next(state);
-- 
2.48.1

On Thu, Jul 10, 2025 at 06:35:14PM +0200, Jens Remus wrote:
> +#ifndef unwind_user_get_reg
> +
> +/**
> + * generic_unwind_user_get_reg - Get register value.
> + * @val: Register value.
> + * @regnum: DWARF register number to obtain the value from.
> + *
> + * Returns zero if successful. Otherwise -EINVAL.
> + */
> +static inline int generic_unwind_user_get_reg(unsigned long *val, int regnum)
> +{
> +	return -EINVAL;
> +}
> +
> +#define unwind_user_get_reg generic_unwind_user_get_reg
> +
> +#endif /* !unwind_user_get_reg */

I believe the traditional way to do this is to give the function the
same name as the define:

#ifndef unwind_user_get_reg
static inline int unwind_user_get_reg(unsigned long *val, int regnum)
{
	return -EINVAL;
}
#define unwind_user_get_reg unwind_user_get_reg
#endif

> +/**
> + * generic_sframe_set_frame_reginfo - Populate info to unwind FP/RA register
> + * from SFrame offset.
> + * @reginfo: Unwind info for FP/RA register.
> + * @offset: SFrame offset value.
> + *
> + * A non-zero offset value denotes a stack offset from CFA and indicates
> + * that the register is saved on the stack. A zero offset value indicates
> + * that the register is not saved.
> + */
> +static inline void generic_sframe_set_frame_reginfo(
> +	struct unwind_user_reginfo *reginfo,
> +	s32 offset)
> +{
> +	if (offset) {
> +		reginfo->loc = UNWIND_USER_LOC_STACK;
> +		reginfo->frame_off = offset;
> +	} else {
> +		reginfo->loc = UNWIND_USER_LOC_NONE;
> +	}
> +}

This just inits the reginfo struct, can we call it sframe_init_reginfo()?

Also the function comment seems completely superfluous as the function
is completely obvious.

Also the signature should match kernel style, something like:

static inline void
sframe_init_reginfo(struct unwind_user_reginfo *reginfo, s32 offset)

> @@ -98,26 +98,57 @@ static int unwind_user_next(struct unwind_user_state *state)
>  
>  
>  	/* Get the Return Address (RA) */
> -	if (frame->ra_off) {
> +	switch (frame->ra.loc) {
> +	case UNWIND_USER_LOC_NONE:
> +		if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
> +			goto done;
> +		ra = user_return_address(task_pt_regs(current));
> +		break;
> +	case UNWIND_USER_LOC_STACK:
> +		if (!frame->ra.frame_off)
> +			goto done;
>  		/* Make sure that the address is word aligned */
>  		shift = sizeof(long) == 4 || compat_fp_state(state) ? 2 : 3;
> -		if ((cfa + frame->ra_off) & ((1 << shift) - 1))
> +		if ((cfa + frame->ra.frame_off) & ((1 << shift) - 1))
>  			goto done;
> -		if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
> +		if (unwind_get_user_long(ra, cfa + frame->ra.frame_off, state))
>  			goto done;
> -	} else {
> -		if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
> +		break;
> +	case UNWIND_USER_LOC_REG:
> +		if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
>  			goto done;
> -		ra = user_return_address(task_pt_regs(current));
> +		if (unwind_user_get_reg(&ra, frame->ra.regnum))
> +			goto done;
> +		break;
> +	default:
> +		WARN_ON_ONCE(1);
> +		goto done;

The default case will never happen, can we make it a BUG()?

>  	}
>  
>  	/* Get the Frame Pointer (FP) */
> -	if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, state))
> +	switch (frame->fp.loc) {
> +	case UNWIND_USER_LOC_NONE:
> +		break;

The UNWIND_USER_LOC_NONE behavior is different here compared to above.
Do we also need UNWIND_USER_LOC_PT_REGS?

> +	case UNWIND_USER_LOC_STACK:
> +		if (!frame->fp.frame_off)
> +			goto done;
> +		if (unwind_get_user_long(fp, cfa + frame->fp.frame_off, state))
> +			goto done;
> +		break;
> +	case UNWIND_USER_LOC_REG:
> +		if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
> +			goto done;

The topmost checking is *really* getting cumbersome, I do hope we can
get rid of that.

> +		if (unwind_user_get_reg(&fp, frame->fp.regnum))
> +			goto done;
> +		break;
> +	default:
> +		WARN_ON_ONCE(1);
>  		goto done;
> +	}

BUG(1) here as well.

>  	state->ip = ra;
>  	state->sp = sp;
> -	if (frame->fp_off)
> +	if (frame->fp.loc != UNWIND_USER_LOC_NONE)
>  		state->fp = fp;

Instead of the extra conditional here, can fp be initialized to zero?
Either at the top of the function or in the RA switch statement?

-- 
Josh

On 17.07.2025 04:01, Josh Poimboeuf wrote:
> On Thu, Jul 10, 2025 at 06:35:14PM +0200, Jens Remus wrote:
>> +#ifndef unwind_user_get_reg
>> +
>> +/**
>> + * generic_unwind_user_get_reg - Get register value.
>> + * @val: Register value.
>> + * @regnum: DWARF register number to obtain the value from.
>> + *
>> + * Returns zero if successful. Otherwise -EINVAL.
>> + */
>> +static inline int generic_unwind_user_get_reg(unsigned long *val, int regnum)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +#define unwind_user_get_reg generic_unwind_user_get_reg
>> +
>> +#endif /* !unwind_user_get_reg */
> 
> I believe the traditional way to do this is to give the function the
> same name as the define:
> 
> #ifndef unwind_user_get_reg
> static inline int unwind_user_get_reg(unsigned long *val, int regnum)
> {
> 	return -EINVAL;
> }
> #define unwind_user_get_reg unwind_user_get_reg
> #endif

Thanks!  I will use use suggestion.

>> +/**
>> + * generic_sframe_set_frame_reginfo - Populate info to unwind FP/RA register
>> + * from SFrame offset.
>> + * @reginfo: Unwind info for FP/RA register.
>> + * @offset: SFrame offset value.
>> + *
>> + * A non-zero offset value denotes a stack offset from CFA and indicates
>> + * that the register is saved on the stack. A zero offset value indicates
>> + * that the register is not saved.
>> + */
>> +static inline void generic_sframe_set_frame_reginfo(
>> +	struct unwind_user_reginfo *reginfo,
>> +	s32 offset)
>> +{
>> +	if (offset) {
>> +		reginfo->loc = UNWIND_USER_LOC_STACK;
>> +		reginfo->frame_off = offset;
>> +	} else {
>> +		reginfo->loc = UNWIND_USER_LOC_NONE;
>> +	}
>> +}
> 
> This just inits the reginfo struct, can we call it sframe_init_reginfo()?
> 
> Also the function comment seems completely superfluous as the function
> is completely obvious.
> 
> Also the signature should match kernel style, something like:
> 
> static inline void
> sframe_init_reginfo(struct unwind_user_reginfo *reginfo, s32 offset)

Ditto.

>> @@ -98,26 +98,57 @@ static int unwind_user_next(struct unwind_user_state *state)
>>  
>>  
>>  	/* Get the Return Address (RA) */
>> -	if (frame->ra_off) {
>> +	switch (frame->ra.loc) {
>> +	case UNWIND_USER_LOC_NONE:
>> +		if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
>> +			goto done;
>> +		ra = user_return_address(task_pt_regs(current));
>> +		break;
>> +	case UNWIND_USER_LOC_STACK:
>> +		if (!frame->ra.frame_off)
>> +			goto done;
>>  		/* Make sure that the address is word aligned */
>>  		shift = sizeof(long) == 4 || compat_fp_state(state) ? 2 : 3;
>> -		if ((cfa + frame->ra_off) & ((1 << shift) - 1))
>> +		if ((cfa + frame->ra.frame_off) & ((1 << shift) - 1))
>>  			goto done;
>> -		if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
>> +		if (unwind_get_user_long(ra, cfa + frame->ra.frame_off, state))
>>  			goto done;
>> -	} else {
>> -		if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
>> +		break;
>> +	case UNWIND_USER_LOC_REG:
>> +		if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
>>  			goto done;
>> -		ra = user_return_address(task_pt_regs(current));
>> +		if (unwind_user_get_reg(&ra, frame->ra.regnum))
>> +			goto done;
>> +		break;
>> +	default:
>> +		WARN_ON_ONCE(1);
>> +		goto done;
> 
> The default case will never happen, can we make it a BUG()?

Whatever Steve and you agree on.  I am new to Kernel development.

>>  	}
>>  
>>  	/* Get the Frame Pointer (FP) */
>> -	if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, state))
>> +	switch (frame->fp.loc) {
>> +	case UNWIND_USER_LOC_NONE:
>> +		break;
> 
> The UNWIND_USER_LOC_NONE behavior is different here compared to above.

See my comments below.

> Do we also need UNWIND_USER_LOC_PT_REGS?

Sorry, I cannot follow.  Do you suggest to rename UNWIND_USER_LOC_REG to
UNWIND_USER_LOC_PT_REGS?

>> +	case UNWIND_USER_LOC_STACK:
>> +		if (!frame->fp.frame_off)
>> +			goto done;
>> +		if (unwind_get_user_long(fp, cfa + frame->fp.frame_off, state))
>> +			goto done;
>> +		break;
>> +	case UNWIND_USER_LOC_REG:
>> +		if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
>> +			goto done;
> 
> The topmost checking is *really* getting cumbersome, I do hope we can
> get rid of that.

Restoring from arbitrary registers is only valid in the topmost frame,
as their values (i.e. task_pt_regs(current)) are only available there.
For other frames only SP, FP, and RA register values are available.

I think this test makes sense.  Is this test really that expensive?

>> +		if (unwind_user_get_reg(&fp, frame->fp.regnum))
>> +			goto done;
>> +		break;
>> +	default:
>> +		WARN_ON_ONCE(1);
>>  		goto done;
>> +	}
> 
> BUG(1) here as well.

Same as for other WARN_ON_ONCE() vs. BUG().

>>  	state->ip = ra;
>>  	state->sp = sp;
>> -	if (frame->fp_off)
>> +	if (frame->fp.loc != UNWIND_USER_LOC_NONE)
>>  		state->fp = fp;
> 
> Instead of the extra conditional here, can fp be initialized to zero?
> Either at the top of the function or in the RA switch statement?

No.  But fp could be initialized to state->fp, so that when it is not
saved and thus not restored it keeps it previous value.

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/

On Thu, Jul 17, 2025 at 01:28:25PM +0200, Jens Remus wrote:
> >>  	}
> >>  
> >>  	/* Get the Frame Pointer (FP) */
> >> -	if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, state))
> >> +	switch (frame->fp.loc) {
> >> +	case UNWIND_USER_LOC_NONE:
> >> +		break;
> > 
> > The UNWIND_USER_LOC_NONE behavior is different here compared to above.
> 
> See my comments below.
> 
> > Do we also need UNWIND_USER_LOC_PT_REGS?
> 
> Sorry, I cannot follow.  Do you suggest to rename UNWIND_USER_LOC_REG to
> UNWIND_USER_LOC_PT_REGS?

I think I completely misunderstood the meaning of UNWIND_USER_LOC_NONE.
Never mind :-)

> >> +	case UNWIND_USER_LOC_STACK:
> >> +		if (!frame->fp.frame_off)
> >> +			goto done;
> >> +		if (unwind_get_user_long(fp, cfa + frame->fp.frame_off, state))
> >> +			goto done;
> >> +		break;
> >> +	case UNWIND_USER_LOC_REG:
> >> +		if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
> >> +			goto done;
> > 
> > The topmost checking is *really* getting cumbersome, I do hope we can
> > get rid of that.
> 
> Restoring from arbitrary registers is only valid in the topmost frame,
> as their values (i.e. task_pt_regs(current)) are only available there.
> For other frames only SP, FP, and RA register values are available.
> 
> I think this test makes sense.  Is this test really that expensive?

ra_off=0 (UNWIND_USER_LOC_NONE) on a !topmost frame should never happen
unless the sframe entry is bad.  But 0 is *far* from the only potential
bad RA offset value.  In the absolute worst case of a 4 byte offset,
there are 4+ billion other possible bad values that can still go
undetected.

So I question the usefulness of those !topmost tests.  And they do add
complexity to the code.

-- 
Josh

On Thu, 17 Jul 2025 13:28:25 +0200
Jens Remus <jremus@linux.ibm.com> wrote:

> >> +	default:
> >> +		WARN_ON_ONCE(1);
> >> +		goto done;  
> > 
> > The default case will never happen, can we make it a BUG()?  
> 
> Whatever Steve and you agree on.  I am new to Kernel development.

Keep the WARN_ON(). Linus has yelled at people for using BUG() when a
WARN_ON() would do.

WARN_ON() is meant for things that should never happen. BUG() is reserved
for places in the kernel that is dangerous to continue.

It's even documented:

  https://docs.kernel.org/process/deprecated.html#bug-and-bug-on

-- Steve

On Wed, 16 Jul 2025 19:01:06 -0700
Josh Poimboeuf <jpoimboe@kernel.org> wrote:

> > +		if (unwind_user_get_reg(&ra, frame->ra.regnum))
> > +			goto done;
> > +		break;
> > +	default:
> > +		WARN_ON_ONCE(1);
> > +		goto done;  
> 
> The default case will never happen, can we make it a BUG()?

Is this really serious enough to crash the system? WARN_ON_ONCE() *is* for
things that will never happen.

The only time I ever use BUG() is if it's too dangerous to continue (like a
function graph trampoline that gets corrupted and has no place to return
to). In general, usage of BUG() should be avoided.

-- Steve

On Wed, Jul 16, 2025 at 11:57:51PM -0400, Steven Rostedt wrote:
> On Wed, 16 Jul 2025 19:01:06 -0700
> Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> 
> > > +		if (unwind_user_get_reg(&ra, frame->ra.regnum))
> > > +			goto done;
> > > +		break;
> > > +	default:
> > > +		WARN_ON_ONCE(1);
> > > +		goto done;  
> > 
> > The default case will never happen, can we make it a BUG()?
> 
> Is this really serious enough to crash the system? WARN_ON_ONCE() *is* for
> things that will never happen.
> 
> The only time I ever use BUG() is if it's too dangerous to continue (like a
> function graph trampoline that gets corrupted and has no place to return
> to). In general, usage of BUG() should be avoided.

This is an unreachable code path, but __builtin_unreachable() is crap
due to undefined behavior.  IMO, BUG() for unreachable paths is cleaner
than WARN_ON_ONCE(), but it doesn't matter much either way I suppose.

-- 
Josh

On Thu, 17 Jul 2025 00:24:47 -0700
Josh Poimboeuf <jpoimboe@kernel.org> wrote:

> > The only time I ever use BUG() is if it's too dangerous to continue (like a
> > function graph trampoline that gets corrupted and has no place to return
> > to). In general, usage of BUG() should be avoided.  
> 
> This is an unreachable code path, but __builtin_unreachable() is crap
> due to undefined behavior.  IMO, BUG() for unreachable paths is cleaner
> than WARN_ON_ONCE(), but it doesn't matter much either way I suppose.

Linus has stated that BUG() should be avoided too. If the code changes in
the future and this suddenly becomes a reachable path, would you rather
have a WARN or a BUG? If you don't have your system set up properly,
the BUG may not even show you why your system crashed.

-- Steve

On Wed, Jul 16, 2025 at 07:01:09PM -0700, Josh Poimboeuf wrote:
> >  	state->ip = ra;
> >  	state->sp = sp;
> > -	if (frame->fp_off)
> > +	if (frame->fp.loc != UNWIND_USER_LOC_NONE)
> >  		state->fp = fp;
> 
> Instead of the extra conditional here, can fp be initialized to zero?
> Either at the top of the function or in the RA switch statement?

So it's been a while since I looked at the original code, but I actually
think there's a bug here.

There's a subtlety in the original code:

	if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, state))
		goto done;

	state->ip = ra;
	state->sp = cfa;
	if (frame->fp_off)
		state->fp = fp;

	arch_unwind_user_next(state);

Note that unlike !frame->ra_off, !frame->fp_off does NOT end the unwind.
That only means the FP offset is unknown for the current frame.  Which
is a perfectly valid condition, e.g. if the function doesn't have frame
pointers or if it's before the prologue.

In that case, the unwind should continue, and state->fp's existing value
should be preserved, as it might already have a valid value from a
previous frame.

So the following is wrong:

	case UNWIND_USER_LOC_STACK:
		if (!frame->fp.frame_off)
			goto done;
		if (unwind_get_user_long(fp, cfa + frame->fp.frame_off, state))
			goto done;
		break;

Instead of having !fp.frame_off stopping the unwind, it should just
break out of the switch statement and keep going.

And that means the following is also wrong:

	state->ip = ra;
	state->sp = sp;
	if (frame->fp.loc != UNWIND_USER_LOC_NONE)
		state->fp = fp;

because state->fp needs to preserved for the STACK+!fp.frame_off case.

So, something like this?

	bool preserve_fp = false;
	...

	/* Get the Frame Pointer (FP) */
	switch (frame->fp.loc) {
	case UNWIND_USER_LOC_NONE:
		preserve_fp = true;
		break;
	case UNWIND_USER_LOC_STACK:
		if (!frame->fp.frame_off) {
			preserve_fp = true;
			break;
		}
	...

	state->ip = ra;
	state->sp = sp;
	if (!preserve_fp)
		state->fp = fp;

BTW, I would suggest renaming "frame_off" to "offset",
"frame->fp.offset" reads better and is more compact.

-- 
Josh

On 17.07.2025 04:50, Josh Poimboeuf wrote:
> On Wed, Jul 16, 2025 at 07:01:09PM -0700, Josh Poimboeuf wrote:
>>>  	state->ip = ra;
>>>  	state->sp = sp;
>>> -	if (frame->fp_off)
>>> +	if (frame->fp.loc != UNWIND_USER_LOC_NONE)
>>>  		state->fp = fp;
>>
>> Instead of the extra conditional here, can fp be initialized to zero?
>> Either at the top of the function or in the RA switch statement?

No, but fp could be initialized to state->fp, so that it retains its
value.

> So it's been a while since I looked at the original code, but I actually
> think there's a bug here.
> 
> There's a subtlety in the original code:
> 
> 	if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, state))
> 		goto done;
> 
> 	state->ip = ra;
> 	state->sp = cfa;
> 	if (frame->fp_off)
> 		state->fp = fp;
> 
> 	arch_unwind_user_next(state);
> 
> Note that unlike !frame->ra_off, !frame->fp_off does NOT end the unwind.
> That only means the FP offset is unknown for the current frame.  Which
> is a perfectly valid condition, e.g. if the function doesn't have frame
> pointers or if it's before the prologue.
> 
> In that case, the unwind should continue, and state->fp's existing value
> should be preserved, as it might already have a valid value from a
> previous frame.

I fully agree with all of the above.

> So the following is wrong:
> 
> 	case UNWIND_USER_LOC_STACK:
> 		if (!frame->fp.frame_off)
> 			goto done;
> 		if (unwind_get_user_long(fp, cfa + frame->fp.frame_off, state))
> 			goto done;
> 		break;
> 
> Instead of having !fp.frame_off stopping the unwind, it should just
> break out of the switch statement and keep going.

If frame->fp.loc is UNWIND_USER_LOC_STACK then frame->fp.frame_off must
have a value != 0.  At least if we keep the original semantic.

We can omit this check, if we assume all producers of frame behave
correctly.  For instance user unwind sframe would not produce that
(see below).  Could it somehow be made a debug-config-only check?

> And that means the following is also wrong:
> 
> 	state->ip = ra;
> 	state->sp = sp;
> 	if (frame->fp.loc != UNWIND_USER_LOC_NONE)
> 		state->fp = fp;
> 
> because state->fp needs to preserved for the STACK+!fp.frame_off case.

unwind user sframe will not produce that:

static inline void
sframe_init_reginfo(struct unwind_user_reginfo *reginfo, s32 offset)
{
	if (offset) {
		reginfo->loc = UNWIND_USER_LOC_STACK;
		reginfo->frame_off = offset;
	} else {
		reginfo->loc = UNWIND_USER_LOC_NONE;
	}
}

> So, something like this?
> 
> 	bool preserve_fp = false;
> 	...
> 
> 	/* Get the Frame Pointer (FP) */
> 	switch (frame->fp.loc) {
> 	case UNWIND_USER_LOC_NONE:
> 		preserve_fp = true;
> 		break;
> 	case UNWIND_USER_LOC_STACK:
> 		if (!frame->fp.frame_off) {
> 			preserve_fp = true;
> 			break;
> 		}
> 	...
> 
> 	state->ip = ra;
> 	state->sp = sp;
> 	if (!preserve_fp)
> 		state->fp = fp;

I don't think all of this is necessary.

frame->fp.loc == UNWIND_USER_LOC_NONE already indicates the state->fp
retains it's previous value.

> BTW, I would suggest renaming "frame_off" to "offset",
> "frame->fp.offset" reads better and is more compact.

Makes sense.  I'll change that.

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/

On Thu, Jul 17, 2025 at 02:07:05PM +0200, Jens Remus wrote:
> On 17.07.2025 04:50, Josh Poimboeuf wrote:
> > So the following is wrong:
> > 
> > 	case UNWIND_USER_LOC_STACK:
> > 		if (!frame->fp.frame_off)
> > 			goto done;
> > 		if (unwind_get_user_long(fp, cfa + frame->fp.frame_off, state))
> > 			goto done;
> > 		break;
> > 
> > Instead of having !fp.frame_off stopping the unwind, it should just
> > break out of the switch statement and keep going.
> 
> If frame->fp.loc is UNWIND_USER_LOC_STACK then frame->fp.frame_off must
> have a value != 0.  At least if we keep the original semantic.
> 
> We can omit this check, if we assume all producers of frame behave
> correctly.  For instance user unwind sframe would not produce that
> (see below).  Could it somehow be made a debug-config-only check?

Ah... the !frame->fp.frame_off check for the UNWIND_USER_LOC_STACK case
completely threw me for a loop.  I was confusing that with
UNWIND_USER_LOC_NONE.  So never mind.

And yes, I think that check has no use and can be removed.

-- 
Josh