Not all architectures have the return address (RA) in user space saved
on the stack on function entry, as x86-64 does due to its CALL
instruction pushing the RA onto the stack.  Architectures/ABIs, such as
s390, also do not necessarily enforce to save the RA in user space on
the stack in the function prologue or even at all, for instance in leaf
functions.

Treat a RA offset from CFA of zero as indication that the RA is not
saved on the stack.  In that case obtain the RA from the RA register.
Allow the SP to be unchanged in the topmost frame, for architectures
where SP at function entry == SP at call site.

Note that treating a RA offset from CFA of zero as indication that
the RA is not saved on the stack additionally allows for architectures,
such as s390, where the frame pointer (FP) may be saved without the RA
being saved as well.  Provided that such architectures represent this
in SFrame by encoding the "missing" RA offset using a padding RA offset
with a value of zero.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
 arch/Kconfig                      |  5 +++++
 include/linux/ptrace.h            |  8 ++++++++
 include/linux/sframe.h            |  4 ++--
 include/linux/unwind_user_types.h |  1 +
 kernel/unwind/sframe.c            | 21 +++++++++++---------
 kernel/unwind/user.c              | 32 ++++++++++++++++++++++---------
 6 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 86eec85cb898..367eaf7e62e0 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -450,6 +450,11 @@ config HAVE_UNWIND_USER_SFRAME
 	bool
 	select UNWIND_USER
 
+config HAVE_USER_RA_REG
+	bool
+	help
+	  The arch passes the return address (RA) in user space in a register.
+
 config SFRAME_VALIDATION
 	bool "Enable .sframe section debugging"
 	depends on HAVE_UNWIND_USER_SFRAME
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 90507d4afcd6..a245c8586673 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -397,6 +397,14 @@ static inline void user_single_step_report(struct pt_regs *regs)
 #define exception_ip(x) instruction_pointer(x)
 #endif
 
+#ifndef user_return_address
+static inline unsigned long user_return_address(const struct pt_regs *regs)
+{
+	WARN_ON_ONCE(1);
+	return 0;
+}
+#endif
+
 extern int task_current_syscall(struct task_struct *target, struct syscall_info *info);
 
 extern void sigaction_compat_abi(struct k_sigaction *act, struct k_sigaction *oact);
diff --git a/include/linux/sframe.h b/include/linux/sframe.h
index b79c5ec09229..e3c6414f1a17 100644
--- a/include/linux/sframe.h
+++ b/include/linux/sframe.h
@@ -33,7 +33,7 @@ extern void sframe_free_mm(struct mm_struct *mm);
 extern int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
 			      unsigned long text_start, unsigned long text_end);
 extern int sframe_remove_section(unsigned long sframe_addr);
-extern int sframe_find(unsigned long ip, struct unwind_user_frame *frame);
+extern int sframe_find(unsigned long ip, struct unwind_user_frame *frame, bool topmost);
 
 static inline bool current_has_sframe(void)
 {
@@ -52,7 +52,7 @@ static inline int sframe_add_section(unsigned long sframe_start, unsigned long s
 	return -ENOSYS;
 }
 static inline int sframe_remove_section(unsigned long sframe_addr) { return -ENOSYS; }
-static inline int sframe_find(unsigned long ip, struct unwind_user_frame *frame) { return -ENOSYS; }
+static inline int sframe_find(unsigned long ip, struct unwind_user_frame *frame, bool topmost) { return -ENOSYS; }
 static inline bool current_has_sframe(void) { return false; }
 
 #endif /* CONFIG_HAVE_UNWIND_USER_SFRAME */
diff --git a/include/linux/unwind_user_types.h b/include/linux/unwind_user_types.h
index 8050a3237a03..adef01698bb3 100644
--- a/include/linux/unwind_user_types.h
+++ b/include/linux/unwind_user_types.h
@@ -35,6 +35,7 @@ struct unwind_user_state {
 	unsigned long fp;
 	struct arch_unwind_user_state arch;
 	enum unwind_user_type type;
+	bool topmost;
 	bool done;
 };
 
diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
index acbf791e713b..5bfaf06e6cd2 100644
--- a/kernel/unwind/sframe.c
+++ b/kernel/unwind/sframe.c
@@ -222,12 +222,8 @@ static __always_inline int __read_fre(struct sframe_section *sec,
 	offset_count--;
 
 	ra_off = sec->ra_off;
-	if (!ra_off) {
-		if (!offset_count--) {
-			dbg_sec_uaccess("zero offset_count, can't find ra_off\n");
-			return -EFAULT;
-		}
-
+	if (!ra_off && offset_count) {
+		offset_count--;
 		UNSAFE_GET_USER_INC(ra_off, cur, offset_size, Efault);
 	}
 
@@ -257,7 +253,8 @@ static __always_inline int __read_fre(struct sframe_section *sec,
 
 static __always_inline int __find_fre(struct sframe_section *sec,
 				      struct sframe_fde *fde, unsigned long ip,
-				      struct unwind_user_frame *frame)
+				      struct unwind_user_frame *frame,
+				      bool topmost)
 {
 	unsigned char fde_type = SFRAME_FUNC_FDE_TYPE(fde->info);
 	struct sframe_fre *fre, *prev_fre = NULL;
@@ -310,6 +307,12 @@ static __always_inline int __find_fre(struct sframe_section *sec,
 		return -EINVAL;
 	fre = prev_fre;
 
+	if ((!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost) && !fre->ra_off) {
+		dbg_sec_uaccess("fde addr 0x%x: zero ra_off\n",
+				fde->start_addr);
+		return -EINVAL;
+	}
+
 	frame->cfa_off = fre->cfa_off;
 	frame->ra_off  = fre->ra_off;
 	frame->fp_off  = fre->fp_off;
@@ -319,7 +322,7 @@ static __always_inline int __find_fre(struct sframe_section *sec,
 	return 0;
 }
 
-int sframe_find(unsigned long ip, struct unwind_user_frame *frame)
+int sframe_find(unsigned long ip, struct unwind_user_frame *frame, bool topmost)
 {
 	struct mm_struct *mm = current->mm;
 	struct sframe_section *sec;
@@ -343,7 +346,7 @@ int sframe_find(unsigned long ip, struct unwind_user_frame *frame)
 	if (ret)
 		goto end;
 
-	ret = __find_fre(sec, &fde, ip, frame);
+	ret = __find_fre(sec, &fde, ip, frame, topmost);
 end:
 	user_read_access_end();
 
diff --git a/kernel/unwind/user.c b/kernel/unwind/user.c
index 45c8c6932ba6..03a6da36192f 100644
--- a/kernel/unwind/user.c
+++ b/kernel/unwind/user.c
@@ -3,6 +3,7 @@
 * Generic interfaces for unwinding user space
 */
 #include <linux/kernel.h>
+#include <linux/ptrace.h>
 #include <linux/sched.h>
 #include <linux/sched/task_stack.h>
 #include <linux/unwind_user.h>
@@ -53,6 +54,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, sp, fp, ra = 0;
+	bool topmost = state->topmost;
 	unsigned int shift;
 
 	if (state->done)
@@ -63,7 +65,7 @@ static int unwind_user_next(struct unwind_user_state *state)
 	} else if (sframe_state(state)) {
 		/* sframe expects the frame to be local storage */
 		frame = &_frame;
-		if (sframe_find(state->ip, frame)) {
+		if (sframe_find(state->ip, frame, topmost)) {
 			if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
 				goto done;
 			frame = &fp_frame;
@@ -86,18 +88,28 @@ static int unwind_user_next(struct unwind_user_state *state)
 
 	/* Get the Stack Pointer (SP) */
 	sp = cfa + frame->sp_val_off;
-	/* Make sure that stack is not going in wrong direction */
-	if (sp <= state->sp)
+	/*
+	 * Make sure that stack is not going in wrong direction.  Allow SP
+	 * to be unchanged for the topmost frame, by subtracting topmost,
+	 * which is either 0 or 1.
+	 */
+	if (sp <= state->sp - topmost)
 		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))
-		goto done;
 
 	/* Get the Return Address (RA) */
-	if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
-		goto done;
+	if (frame->ra_off) {
+		/* 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))
+			goto done;
+		if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
+			goto done;
+	} else {
+		if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
+			goto done;
+		ra = user_return_address(task_pt_regs(current));
+	}
 
 	/* Get the Frame Pointer (FP) */
 	if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, state))
@@ -110,6 +122,7 @@ static int unwind_user_next(struct unwind_user_state *state)
 
 	arch_unwind_user_next(state);
 
+	state->topmost = false;
 	return 0;
 
 done:
@@ -140,6 +153,7 @@ static int unwind_user_start(struct unwind_user_state *state)
 	state->ip = instruction_pointer(regs);
 	state->sp = user_stack_pointer(regs);
 	state->fp = frame_pointer(regs);
+	state->topmost = true;
 
 	arch_unwind_user_init(state, regs);
 
-- 
2.48.1

On Thu, Jul 10, 2025 at 06:35:13PM +0200, Jens Remus wrote:
> +++ b/arch/Kconfig
> @@ -450,6 +450,11 @@ config HAVE_UNWIND_USER_SFRAME
>  	bool
>  	select UNWIND_USER
>  
> +config HAVE_USER_RA_REG
> +	bool
> +	help
> +	  The arch passes the return address (RA) in user space in a register.

How about "HAVE_UNWIND_USER_RA_REG" so it matches the existing
namespace?

> @@ -310,6 +307,12 @@ static __always_inline int __find_fre(struct sframe_section *sec,
>  		return -EINVAL;
>  	fre = prev_fre;
>  
> +	if ((!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost) && !fre->ra_off) {
> +		dbg_sec_uaccess("fde addr 0x%x: zero ra_off\n",
> +				fde->start_addr);
> +		return -EINVAL;
> +	}

The topmost frame doesn't necessarily (or even likely) come from before
the prologue, or from a leaf function, so this check would miss the case
where a non-leaf function wrongly has !ra_off after its prologue.

Which in reality is probably fine, as there are other guardrails in
place to catch such bad sframe data.

But then do we think the !ra_off check is still worth the effort?  It
would be simpler to just always assume !ra_off is valid for the
CONFIG_HAVE_USER_RA_REG case.

I think I prefer the simplicity of removing the check, as the error
would be rare, and corrupt sframe would be caught in other ways.

> @@ -86,18 +88,28 @@ static int unwind_user_next(struct unwind_user_state *state)
>  
>  	/* Get the Stack Pointer (SP) */
>  	sp = cfa + frame->sp_val_off;
> -	/* Make sure that stack is not going in wrong direction */
> -	if (sp <= state->sp)
> +	/*
> +	 * Make sure that stack is not going in wrong direction.  Allow SP
> +	 * to be unchanged for the topmost frame, by subtracting topmost,
> +	 * which is either 0 or 1.
> +	 */
> +	if (sp <= state->sp - topmost)
>  		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))
> -		goto done;
>  
>  	/* Get the Return Address (RA) */
> -	if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
> -		goto done;
> +	if (frame->ra_off) {
> +		/* 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))
> +			goto done;
> +		if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
> +			goto done;
> +	} else {
> +		if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
> +			goto done;

I think this check is redundant with the one in __find_fre()?

-- 
Josh

On 17.07.2025 01:01, Josh Poimboeuf wrote:
> On Thu, Jul 10, 2025 at 06:35:13PM +0200, Jens Remus wrote:
>> +++ b/arch/Kconfig
>> @@ -450,6 +450,11 @@ config HAVE_UNWIND_USER_SFRAME
>>  	bool
>>  	select UNWIND_USER
>>  
>> +config HAVE_USER_RA_REG
>> +	bool
>> +	help
>> +	  The arch passes the return address (RA) in user space in a register.
> 
> How about "HAVE_UNWIND_USER_RA_REG" so it matches the existing
> namespace?

Ok.  I am open to any improvements.

>> @@ -310,6 +307,12 @@ static __always_inline int __find_fre(struct sframe_section *sec,
>>  		return -EINVAL;
>>  	fre = prev_fre;
>>  
>> +	if ((!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost) && !fre->ra_off) {
>> +		dbg_sec_uaccess("fde addr 0x%x: zero ra_off\n",
>> +				fde->start_addr);
>> +		return -EINVAL;
>> +	}
> 
> The topmost frame doesn't necessarily (or even likely) come from before
> the prologue, or from a leaf function, so this check would miss the case
> where a non-leaf function wrongly has !ra_off after its prologue.
>
> Which in reality is probably fine, as there are other guardrails in
> place to catch such bad sframe data.

On s390 the compiler may intermingle prologue instructions with function
body instructions.  As a result !ra_off is valid as long as the RA
register value from function entry has not been destroyed.  Furthermore
the compiler may decide to save RA only if it is actually about to
destroy the RA register value (e.g. due to a function call).

Note that it is valid to restore the RA register value anywhere in a
function and represent that in SFrame.  Following is an example from
Glibc libc.so psignal() on s390x.  The RA rule changes back to "u"
(= undefined; !ra_off) multiple times, whenever the RA register has
its value from function entry again.

  func idx [589]: pc = 0x6c530, size = 250 bytes <psignal>
  STARTPC         CFA       FP        RA
  000000000006c530  sp+160    u         u
  000000000006c536  sp+160    c-72      c-48
  000000000006c53c  sp+328    c-72      c-48
  000000000006c5b4  sp+160    u         u
  000000000006c5b6  sp+328    c-72      c-48
  000000000006c60c  sp+160    u         u
  000000000006c60e  sp+328    c-72      c-48

> But then do we think the !ra_off check is still worth the effort?  It
> would be simpler to just always assume !ra_off is valid for the
> CONFIG_HAVE_USER_RA_REG case.

It is only valid in the topmost frame.  Otherwise something is wrong.

> I think I prefer the simplicity of removing the check, as the error
> would be rare, and corrupt sframe would be caught in other ways.

But I am fine with removing it in user unwind sframe (above), as
user unwind performs the same check (see below).

>> @@ -86,18 +88,28 @@ static int unwind_user_next(struct unwind_user_state *state)
>>  
>>  	/* Get the Stack Pointer (SP) */
>>  	sp = cfa + frame->sp_val_off;
>> -	/* Make sure that stack is not going in wrong direction */
>> -	if (sp <= state->sp)
>> +	/*
>> +	 * Make sure that stack is not going in wrong direction.  Allow SP
>> +	 * to be unchanged for the topmost frame, by subtracting topmost,
>> +	 * which is either 0 or 1.
>> +	 */
>> +	if (sp <= state->sp - topmost)
>>  		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))
>> -		goto done;
>>  
>>  	/* Get the Return Address (RA) */
>> -	if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
>> -		goto done;
>> +	if (frame->ra_off) {
>> +		/* 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))
>> +			goto done;
>> +		if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
>> +			goto done;
>> +	} else {
>> +		if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
>> +			goto done;
> 
> I think this check is redundant with the one in __find_fre()?

Correct.  This one needs to stay, as it is at the topmost level (user
unwind checks the unwind info obtained from user unwind sframe or any
other method).

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 17.07.2025 13:09, Jens Remus wrote:
> On 17.07.2025 01:01, Josh Poimboeuf wrote:
>> On Thu, Jul 10, 2025 at 06:35:13PM +0200, Jens Remus wrote:
>>> +++ b/arch/Kconfig
>>> @@ -450,6 +450,11 @@ config HAVE_UNWIND_USER_SFRAME
>>>  	bool
>>>  	select UNWIND_USER
>>>  
>>> +config HAVE_USER_RA_REG
>>> +	bool
>>> +	help
>>> +	  The arch passes the return address (RA) in user space in a register.
>>
>> How about "HAVE_UNWIND_USER_RA_REG" so it matches the existing
>> namespace?
> 
> Ok.  I am open to any improvements.

Thinking about this again I realized that the config option actually
serves two purposes:

1. Enable code (e.g. unwind user) to determine the presence of the new
   user_return_address().  That is where I derived the name from.
2. Enable unwind user (sframe) to behave differently, if an architecture
   has/uses a RA register (unlike x86, which solely uses the stack).

I think the primary notion is that an architecture has/uses a register
for the return address and thus provides user_return_address().  What
consumers such as unwind user do with that info is secondary.

Thoughts?

Thanks and 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 Fri, Jul 18, 2025 at 10:28:32AM +0200, Jens Remus wrote:
> On 17.07.2025 13:09, Jens Remus wrote:
> > On 17.07.2025 01:01, Josh Poimboeuf wrote:
> >> On Thu, Jul 10, 2025 at 06:35:13PM +0200, Jens Remus wrote:
> >>> +++ b/arch/Kconfig
> >>> @@ -450,6 +450,11 @@ config HAVE_UNWIND_USER_SFRAME
> >>>  	bool
> >>>  	select UNWIND_USER
> >>>  
> >>> +config HAVE_USER_RA_REG
> >>> +	bool
> >>> +	help
> >>> +	  The arch passes the return address (RA) in user space in a register.
> >>
> >> How about "HAVE_UNWIND_USER_RA_REG" so it matches the existing
> >> namespace?
> > 
> > Ok.  I am open to any improvements.
> 
> Thinking about this again I realized that the config option actually
> serves two purposes:
> 
> 1. Enable code (e.g. unwind user) to determine the presence of the new
>    user_return_address().  That is where I derived the name from.
> 2. Enable unwind user (sframe) to behave differently, if an architecture
>    has/uses a RA register (unlike x86, which solely uses the stack).

The sframe CONFIG_HAVE_USER_RA_REG check is redundant with the
unwind_user one, no?  I'm thinking it's better for sframe to just decode
the entry as it is, and then let unwind_user validate things.

> I think the primary notion is that an architecture has/uses a register
> for the return address and thus provides user_return_address().  What
> consumers such as unwind user do with that info is secondary.
> 
> Thoughts?

user_return_address() only has the single user, and is not all that
generically useful anyway (e.g., it warns on x86), so let's keep it
encapsulated in include/linux/unwind_user.h and give it the
"unwind_user" prefix.

Also, "RA_REG" is a bit ambiguous, it sounds almost like that other
option which spills RA to another register.  Conceptually, it's a link
register, so can we rename that to CONFIG_HAVE_UNWIND_USER_LINK_REG and
unwind_user_get_link_reg() or so?

Similarly, CONFIG_HAVE_UNWIND_USER_LOC_REG isn't that descriptive, how
about CONFIG_HAVE_UNWIND_USER_LINK_REG_SPILL?

Also we can get rid of the '#define func_name func_name' things and just
guard those functions with their corresponding CONFIG options in
inclide/linux/unwind_user.h.

Also those two functions should have similar naming and prototypes.

For example, in include/linux/unwind_user.h:

#ifndef CONFIG_HAVE_UNWIND_USER_LINK_REG
int unwind_user_get_link_reg(unsigned long *val)
{
	WARN_ON_ONCE(1);
	return -EINVAL;
}
#endif

#ifndef CONFIG_HAVE_UNWIND_USER_LINK_REG_SPILL
int unwind_user_get_reg(unsigned long *val, unsigned int regnum)
{
	WARN_ON_ONCE(1);
	return -EINVAL;
}
#endif

Then the code can be simplified (assuming no topmost checks):

	/* Get the Return Address (RA) */
	switch (frame->ra.loc) {
	case UNWIND_USER_LOC_NONE:
		if (unwind_user_get_link_reg(&ra))
			goto done;
		break;
	...
	case UNWIND_USER_LOC_REG:
		if (unwind_user_get_reg(&ra, frame->ra.regnum))
			goto done;
		break;
	...

-- 
Josh

Hello Josh,

thank you very much for your valuable feedback!

On 18.07.2025 18:59, Josh Poimboeuf wrote:
> On Fri, Jul 18, 2025 at 10:28:32AM +0200, Jens Remus wrote:
>> On 17.07.2025 13:09, Jens Remus wrote:
>>> On 17.07.2025 01:01, Josh Poimboeuf wrote:
>>>> On Thu, Jul 10, 2025 at 06:35:13PM +0200, Jens Remus wrote:
>>>>> +++ b/arch/Kconfig
>>>>> @@ -450,6 +450,11 @@ config HAVE_UNWIND_USER_SFRAME
>>>>>  	bool
>>>>>  	select UNWIND_USER
>>>>>  
>>>>> +config HAVE_USER_RA_REG
>>>>> +	bool
>>>>> +	help
>>>>> +	  The arch passes the return address (RA) in user space in a register.
>>>>
>>>> How about "HAVE_UNWIND_USER_RA_REG" so it matches the existing
>>>> namespace?
>>>
>>> Ok.  I am open to any improvements.
>>
>> Thinking about this again I realized that the config option actually
>> serves two purposes:
>>
>> 1. Enable code (e.g. unwind user) to determine the presence of the new
>>    user_return_address().  That is where I derived the name from.
>> 2. Enable unwind user (sframe) to behave differently, if an architecture
>>    has/uses a RA register (unlike x86, which solely uses the stack).
> 
> The sframe CONFIG_HAVE_USER_RA_REG check is redundant with the
> unwind_user one, no?  I'm thinking it's better for sframe to just decode
> the entry as it is, and then let unwind_user validate things.

Ok.  Makes sense.

>> I think the primary notion is that an architecture has/uses a register
>> for the return address and thus provides user_return_address().  What
>> consumers such as unwind user do with that info is secondary.
>>
>> Thoughts?
> 
> user_return_address() only has the single user, and is not all that
> generically useful anyway (e.g., it warns on x86), so let's keep it
> encapsulated in include/linux/unwind_user.h and give it the
> "unwind_user" prefix.

Ok.  I was hesitating to add it to ptrace.h.  Given it falls into the
same category as user_stack_pointer() and frame_pointer() I was
astonished it did not yet exist.  But given unwind user would be the
single user I agree that it is better to do as you suggest.

> Also, "RA_REG" is a bit ambiguous, it sounds almost like that other
> option which spills RA to another register.  Conceptually, it's a link
> register, so can we rename that to CONFIG_HAVE_UNWIND_USER_LINK_REG and
> unwind_user_get_link_reg() or so?

The terminology in DWARF and SFrame is return address (RA) register.
AArch64 uses link register (LR). s390 uses RA register. x86 uses return
address.  While I am open to use your suggestion, I wonder what others
would prefer.

In fact the whole option is not required, if using '#define fname fname'
instead (that you suggest not to down below).  user_unwind.c would then
contain the following as default for architectures that do not define
their custom implementation (or link_reg instead of ra_reg):

#ifndef unwind_user_get_ra_reg
int unwind_user_get_ra_reg(unsigned long *val)
{
	WARN_ON_ONCE(1);
	return -EINVAL;
}
#define unwind_user_get_ra_reg unwind_user_get_ra_reg
#endif

The commit subject should better be changed to one of the following
(with the commit message reworded accordingly):

unwind_user: Enable archs that have a RA register
  or
unwind_user: Enable archs that have a link register

> Similarly, CONFIG_HAVE_UNWIND_USER_LOC_REG isn't that descriptive, how
> about CONFIG_HAVE_UNWIND_USER_LINK_REG_SPILL?

Are you perhaps mixing up two of the new config options introduced by
this and the subsequent patch?

HAVE_USER_RA_REG (introduced by this patch): Indicates the architecture
has/uses a RA/LR register.  I would be fine to rename this to
"CONFIG_HAVE_UNWIND_USER_LINK_REG" (as you suggest), provided "link
register" is the terminology we can all agree on, although DWARF and
SFrame use "return address register".  Otherwise
"CONFIG_HAVE_UNWIND_USER_RA_REG" or even omit, if there is no need for
the option at all.

CONFIG_HAVE_UNWIND_USER_LOC_REG (introduced by the subsequent patch):
Indicates that the architecture may save registers in other registers.
Those support UNWIND_USER_LOC_REG (location register) in addition to
UNWIND_USER_LOC_STACK (location stack).  Note that this applies to
both FP and RA.

> Also we can get rid of the '#define func_name func_name' things and just
> guard those functions with their corresponding CONFIG options in
> inclide/linux/unwind_user.h.
> 
> Also those two functions should have similar naming and prototypes.
> 
> For example, in include/linux/unwind_user.h:
> 
> #ifndef CONFIG_HAVE_UNWIND_USER_LINK_REG
> int unwind_user_get_link_reg(unsigned long *val)
> {
> 	WARN_ON_ONCE(1);
> 	return -EINVAL;
> }
> #endif
> 
> #ifndef CONFIG_HAVE_UNWIND_USER_LINK_REG_SPILL
> int unwind_user_get_reg(unsigned long *val, unsigned int regnum)
> {
> 	WARN_ON_ONCE(1);
> 	return -EINVAL;
> }
> #endif

Is a config option actually required (see above)?  The reason I added
them was to perform checks, that you suggest to omit.  Your below code
suggestion would no longer required the config options at all.  What
is preferable?  The config option would ensure a build error if an
architecture enables the option without providing the arch-specific
implementations.

> Then the code can be simplified (assuming no topmost checks):
> 
> 	/* Get the Return Address (RA) */
> 	switch (frame->ra.loc) {
> 	case UNWIND_USER_LOC_NONE:
> 		if (unwind_user_get_link_reg(&ra))
> 			goto done;
> 		break;
> 	...
> 	case UNWIND_USER_LOC_REG:
> 		if (unwind_user_get_reg(&ra, frame->ra.regnum))
> 			goto done;
> 		break;
> 	...

Thanks and 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/