Re: [PATCH v5 8/8] unwind: arm64: Use sframe to unwind interrupt frames

Jens Remus
2026-05-04T08:48:17Z

Hello Mark,

I mostly have comments regarding your the SFrame related remarks.

On 5/1/2026 6:46 PM, Mark Rutland wrote:
> Thanks for putting this together. I think this is looking pretty good.
> However, there are some things that aren't quite right and need some
> work, which I've commented on below.

> (2) To make unwinding generally possible, we'll need to annotate some
> assembly functions as unwindable. We'll need to do that for string
> routines under lib/, and probably some crypto code, but we don't
> need to do that for most code in head.S, entry.S, etc.
>
> The vast majority of relevant assembly functions are leaf functions
> (where the return address is never moved out of the LR), so we can
> probably get away with a simple annotation for those that avoids the
> need for open-coded CFI directives everywhere.

Wrapping them in .cfi_startproc ... .cfi_endproc should do. For instance
by extending SYM_FUNC_START() and SYM_FUNC_END() or introducing flavors
that do. Or where you thinking of something else?

> On Tue, Apr 28, 2026 at 06:36:43PM +0000, Dylan Hatch wrote:

>> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h

>> @@ -21,6 +21,8 @@ struct stack_info {
>> *
>> * @fp: The fp value in the frame record (or the real fp)
>> * @pc: The lr value in the frame record (or the real lr)
>> + * @sp: The sp value at the call site of the current function.
>> + * @unreliable: Stacktrace is unreliable.
>> *
>> * @stack: The stack currently being unwound.
>> * @stacks: An array of stacks which can be unwound.
>> @@ -29,7 +31,11 @@ struct stack_info {
>> struct unwind_state {
>> unsigned long fp;
>> unsigned long pc;
>> +#ifdef CONFIG_HAVE_UNWIND_KERNEL_SFRAME
>> + unsigned long sp;
>> +#endif
>
> As this is only used by the kernel unwinder (and not the hyp unwinder),
> this should live in struct kunwind_state in stacktrace.c.
>
> That said, for unwinding across exception boundaries we should not need
> this, as the SP value will be in the pt_regs. If we only use SFrame for
> the exception boundary case, we can remove this entirely. I would
> strongly prefer that we do that.

>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c

>> +/*
>> + * Unwind to the next frame according to sframe.
>> + */
>> +static __always_inline int
>> +unwind_next_frame_sframe(struct kunwind_state *state)
>> +{
>> + struct unwind_frame frame;
>> + unsigned long cfa, fp, ra;
>> + enum kunwind_source source = KUNWIND_SOURCE_FRAME;
>> + struct pt_regs *regs = state->regs;
>> +
>> + int err;
>
> As above, we should only use this for unwinding from the regs, after a
> KUNWIND_SOURCE_REGS_PC step.
>
> With that in mind, it would be good to:
>
> (1) Rename this to something like kunwind_next_regs_sframe(). Note
> 'kunwind' rather than 'unwind' for consistency with the rest of this
> file.
>
> (2) Add the following sanity checks:
>
> if (WARN_ON_ONCE(state->source != KUNWIND_SOURCE_REGS_PC))
> return -EINVAL;
> if (WARN_ON_ONCE(!state->regs))
> return -EINVAL;
>
> ... which will also allow the code below to be simplified.
>
>> +
>> + /* FP/SP alignment 8 bytes */
>> + if (state->common.fp & 0x7 || state->common.sp & 0x7)
>> + return -EINVAL;
>> +
>> + /*
>> + * Most/all outermost functions are not visible to sframe. So, check for
>> + * a meta frame record if the sframe lookup fails.
>> + */
>> + err = sframe_find_kernel(state->common.pc, &frame);
>> + if (err)
>> + return kunwind_next_frame_record_meta(state);
>> +
>> + if (frame.outermost)
>> + return -ENOENT;
>
> I don't think we ever expect an outermost frame within the kernel. We
> haven't added any annotations for that, and we expect to unwind all the
> way to a FRAME_META_TYPE_FINAL frame.
>
> We cannot fall back to kunwind_next_frame_record_meta() here. We don't
> know that the next frame is a meta frame (and this results in a warning
> noted above), and we don't know the result is going to be reliable if we
> don't have SFrame data, so the right thing to do is return an error.
>
> I think this should be:
>
> /*
> * A kernel unwind should always end at a FRAME_META_TYPE_FINAL
> * frame. There should be no outermost frames within the kernel.
> */
> if (frame.outermost)
> return -EINVAL;

Makes sense.

>
> err = sframe_find_kernel(state->common.pc, &frame);
> if (err)
> return -EINVAL;
>
>> + /* Get the Canonical Frame Address (CFA) */
>> + switch (frame.cfa.rule) {
>> + case UNWIND_CFA_RULE_SP_OFFSET:
>> + cfa = state->common.sp;

IIUC you suggest this to be changed as follows?

cfa = regs->regs[31];

>> + break;
>> + case UNWIND_CFA_RULE_FP_OFFSET:
>> + if (state->common.fp < state->common.sp)
>> + return -EINVAL;
>> + cfa = state->common.fp;
>> + break;
>> + case UNWIND_CFA_RULE_REG_OFFSET:
>> + case UNWIND_CFA_RULE_REG_OFFSET_DEREF:
>> + /* regs only available in topmost/interrupt frame */
>> + if (!regs || frame.cfa.regnum > 30)
>> + return -EINVAL;
>> + cfa = regs->regs[frame.cfa.regnum];
>> + break;
>
> Do we ever expect to see UNWIND_CFA_RULE_REG_OFFSET or
> UNWIND_CFA_RULE_REG_OFFSET_DEREF in practice for kernel code?

No. Those can only occur with SFrame V3 flexible FDE, which are
currently not generated by GNU assembler for arm64/aarch64, and thus
could be omitted in the arm64-specific kernel sframe unwinder:

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/config/tc-aarch64.h;hb=binutils-2_46#l342

I must admit that while reviewing I thought it would be future-proof to
have support for rules that can only be represented with SFrame V3
flexible FDE, even if they are currently not used on arm64. Ideally
kunwind_next_sframe() could be made common, if another architecture
would implement kernel unwinding using sframe.

>
>> + default:
>> + WARN_ON_ONCE(1);
>> + return -EINVAL;
>> + }
>> + cfa += frame.cfa.offset;
>> +
>> + /*
>> + * CFA typically points to a higher address than RA or FP, so don't
>> + * consume from the stack when we read it.
>> + */
>> + if (frame.cfa.rule & UNWIND_RULE_DEREF &&
>> + !get_word(&state->common, &cfa))
>> + return -EINVAL;
>
> Per the switch above, this could only be
> UNWIND_CFA_RULE_REG_OFFSET_DEREF. As above, do we ever expect to
> encounter that in practice for kernel code?

No. See above.

>
>> +
>> + /* CFA alignment 8 bytes */
>> + if (cfa & 0x7)
>> + return -EINVAL;
>
> If the CFA is the SP upon entry to the function, then per AAPCS64 rules
> it should be aligned to 16 bytes. Otherwise, where has this 8 byte
> alignment requirement come from? Does SFrame mandate that?

That originates from the common unwind user logic (see
kernel/unwind/user.c, unwind_user_next_common()), which currently
assumes 8-byte/4-byte SP alignment for all 64-bit/32-bit architectures.

So checking for 16-byte alignment here would make sense.

>
>> +
>> + /* Get the Return Address (RA) */
>> + switch (frame.ra.rule) {
>> + case UNWIND_RULE_RETAIN:
>> + /* regs only available in topmost/interrupt frame */
>> + if (!regs)
>> + return -EINVAL;
>> + ra = regs->regs[30];
>> + source = KUNWIND_SOURCE_REGS_LR;
>> + break;
>> + /* UNWIND_USER_RULE_CFA_OFFSET not implemented on purpose */

Nit: s/UNWIND_USER_RULE_CFA_OFFSET/UNWIND_RULE_CFA_OFFSET/

>
> It would be better for the comment to say *why* that's not implemented.
>
> I assume that's because UNWIND_USER_RULE_CFA_OFFSET would mean that the return
> address is a stack address, and that's obviously not legitimate.

That and SFrame V3 currently cannot represent FP/RA as CFA + offset
(i.e. UNWIND_RULE_CFA_OFFSET; .cfi_val_offset FP/RA).

The comment originates from the common unwind user logic (see
kernel/unwind/user.c). I am open to improve that. What about:

/*
* UNWIND_RULE_CFA_OFFSET not implemented on purpose, as a stack
* address cannot be a legitimate return address value. It is
* also not used (e.g. not represented in sframe).
*/

>
>> + case UNWIND_RULE_CFA_OFFSET_DEREF:
>> + ra = cfa + frame.ra.offset;
>> + break;
>> + case UNWIND_RULE_REG_OFFSET:
>> + case UNWIND_RULE_REG_OFFSET_DEREF:
>> + /* regs only available in topmost/interrupt frame */
>> + if (!regs)
>> + return -EINVAL;
>> + ra = regs->regs[frame.cfa.regnum];
>> + ra += frame.ra.offset;
>> + break;
>
> Do we ever expect UNWIND_RULE_REG_OFFSET or UNWIND_RULE_REG_OFFSET_DEREF
> in practice for kernel code?

No. See above (SFrame V3 flexible FDE).

>
> I don't think we expect UNWIND_RULE_REG_OFFSET unless that's sometimes used
> instead of UNWIND_RULE_RETAIN to express that the return address is in x30
> (with zero offset).

No. Unless there would be nonsense .cfi_register 30, 30, which would
require SFrame V3 flexible FDE to be represented.

@Indu: We may consider to treat .cfi_register <reg>, <reg> (for FP/RA)
like .cfi_restore <reg> in the GNU assembler?

>
> Similarly, if the address is on the stack it should be in a frame
> record. Would we ever expect UNWIND_RULE_REG_OFFSET_DEREF rather than
> UNWIND_RULE_CFA_OFFSET_DEREF?

No. See above (SFrame V3 flexible FDE).

>
>> + default:
>> + WARN_ON_ONCE(1);
>> + return -EINVAL;
>> + }
>> +
>> + /* Get the Frame Pointer (FP) */
>> + switch (frame.fp.rule) {
>> + case UNWIND_RULE_RETAIN:
>> + fp = state->common.fp;
>> + break;
>> + /* UNWIND_USER_RULE_CFA_OFFSET not implemented on purpose */
>
> As for RA, the comment should explain why that's not implemented.

I am open to improve the comment in the the common unwind user logic.
What about:

/*
* UNWIND_RULE_CFA_OFFSET not implemented on purpose, as it is
* not used (e.g. not represented in sframe).
*/

>
>> + case UNWIND_RULE_CFA_OFFSET_DEREF:
>> + fp = cfa + frame.fp.offset;
>> + break;
>> + case UNWIND_RULE_REG_OFFSET:
>> + case UNWIND_RULE_REG_OFFSET_DEREF:
>> + /* regs only available in topmost/interrupt frame */
>> + if (!regs)
>> + return -EINVAL;
>> + fp = regs->regs[frame.fp.regnum];
>> + fp += frame.fp.offset;
>> + break;

Likewise neither UNWIND_RULE_REG_OFFSET nor UNWIND_RULE_REG_OFFSET_DEREF
can currently occur on arm64. See above (SFrame V3 flexible FDE).

>> + default:
>> + WARN_ON_ONCE(1);
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * Consume RA and FP from the stack. The frame record puts FP at a lower
>> + * address than RA, so we always read FP first.
>> + */
>> + if (frame.fp.rule & UNWIND_RULE_DEREF &&
>> + !get_word(&state->common, &fp))
>> + return -EINVAL;
>
> Why is this get_word() rather than get_consume_word()?
>
>> +
>> + if (frame.ra.rule & UNWIND_RULE_DEREF &&
>> + get_consume_word(&state->common, &ra))
>> + return -EINVAL;
>> +
>> + state->common.pc = ra;
>> + state->common.sp = cfa;
>
> As above, the SP can be removed.
>
>> + state->common.fp = fp;
>> +
>> + state->source = source;
>> +
>> + return 0;
>> +}
Regards,
Jens
--
Jens Remus
Linux on Z Development (D3303)
jremus@de.ibm.com / jremus@linux.ibm.com

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