Re: [PATCH v13 02/14] unwind_user: Add frame pointer support

[Jens Remus <jremus@linux.ibm.com>]

On 08.07.2025 03:22, Steven Rostedt wrote:
> From: Josh Poimboeuf <jpoimboe@kernel.org>
> 
> Add optional support for user space frame pointer unwinding.  If
> supported, the arch needs to enable CONFIG_HAVE_UNWIND_USER_FP and
> define ARCH_INIT_USER_FP_FRAME.
> 
> By encoding the frame offsets in struct unwind_user_frame, much of this
> code can also be reused for future unwinder implementations like sframe.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> Co-developed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

> diff --git a/kernel/unwind/user.c b/kernel/unwind/user.c

> @@ -6,13 +6,71 @@
>  #include <linux/sched.h>
>  #include <linux/sched/task_stack.h>
>  #include <linux/unwind_user.h>
> +#include <linux/uaccess.h>
> +
> +static struct unwind_user_frame fp_frame = {
> +	ARCH_INIT_USER_FP_FRAME
> +};
> +
> +static inline bool fp_state(struct unwind_user_state *state)
> +{
> +	return IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP) &&
> +	       state->type == UNWIND_USER_TYPE_FP;
> +}
>  
>  #define for_each_user_frame(state) \
>  	for (unwind_user_start(state); !(state)->done; unwind_user_next(state))
>  
>  static int unwind_user_next(struct unwind_user_state *state)
>  {
> -	/* no implementation yet */
> +	struct unwind_user_frame *frame;
> +	unsigned long cfa = 0, fp, ra = 0;
> +	unsigned int shift;
> +
> +	if (state->done)
> +		return -EINVAL;
> +
> +	if (fp_state(state))
> +		frame = &fp_frame;
> +	else
> +		goto done;
> +
> +	if (frame->use_fp) {
> +		if (state->fp < state->sp)

		if (state->fp <= state->sp)

I meanwhile came to the conclusion that for architectures, such as s390,
where SP at function entry == SP at call site, the FP may be equal to
the SP.  At least for the brief period where the FP has been setup and
stack allocation did not yet take place.  For most architectures this
can probably only occur in the topmost frame.  For s390 the FP is setup
after static stack allocation, so --fno-omit-frame-pointer would enforce
FP==SP in any frame that does not perform dynamic stack allocation.

> +			goto done;
> +		cfa = state->fp;
> +	} else {
> +		cfa = state->sp;
> +	}
> +
> +	/* Get the Canonical Frame Address (CFA) */
> +	cfa += frame->cfa_off;
> +
> +	/* stack going in wrong direction? */
> +	if (cfa <= state->sp)
> +		goto done;
> +
> +	/* Make sure that the address is word aligned */
> +	shift = sizeof(long) == 4 ? 2 : 3;
> +	if ((cfa + frame->ra_off) & ((1 << shift) - 1))
> +		goto done;

Do all architectures/ABI mandate register stack save slots to be aligned?
s390 does.

> +
> +	/* Find the Return Address (RA) */
> +	if (get_user(ra, (unsigned long *)(cfa + frame->ra_off)))
> +		goto done;
> +

Why not validate the FP stack save slot address as well?

> +	if (frame->fp_off && get_user(fp, (unsigned long __user *)(cfa + frame->fp_off)))
> +		goto done;
> +
> +	state->ip = ra;
> +	state->sp = cfa;
> +	if (frame->fp_off)
> +		state->fp = fp;
> +
> +	return 0;
> +
> +done:
> +	state->done = true;
>  	return -EINVAL;
>  }

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/