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

[Dylan Hatch]

On Fri, Apr 17, 2026 at 8:45 AM Jens Remus <jremus@xxxxxxxxxxxxx> wrote:
>
> > +     case UNWIND_CFA_RULE_FP_OFFSET:
> > +             if (state->common.fp < state->common.sp)
> > +                     return -EINVAL;
>
> I wonder whether that check is valid in kernel?  Looking at
> call_on_irq_stack() saving SP in FP and then loading SP with the IRQ SP.
> Is that condition always true then?

Good catch. I will double-check this.

>
> > +             cfa = state->common.fp;
> > +             break;
> > +     case UNWIND_CFA_RULE_REG_OFFSET:
> > +     case UNWIND_CFA_RULE_REG_OFFSET_DEREF:
> > +             if (!regs)
>
>                 if (!regs || frame.cfa.regnum > 30)
>
> > +                     return -EINVAL;
> > +             cfa = regs->regs[frame.cfa.regnum];
>
> In unwind user this is guarded by a topmost frame check, as arbitrary
> registers are otherwise not available.  Isn't this necessary in the
> kernel case?

It is necessary, though as you point out the way I wrote the check is
not as obvious as it probably should be.

The saved state->regs is set when the current frame is recovered from
the saved PC of a struct pt_regs, and then immediately set back to
NULL after the next frame has been recovered. In other words, the
state->regs is only ever set when it is relevant to the current frame,
which occurs when state->source == KUNWIND_SOURCE_REGS_PC. This only
happens when the topmost frame is recovered from a pt_regs, or when a
pt_regs is recovered from the stack due to an interrupt.

I can make this more readable by adding an explicit check for
KUNWIND_SOURCE_REGS_PC in addition to state->regs != NULL.

>
> > +             break;
> > +     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;
> > +
> > +     /* CFA alignment 8 bytes */
> > +     if (cfa & 0x7)
> > +             return -EINVAL;
> > +
> > +     /* Get the Return Address (RA) */
> > +     switch (frame.ra.rule) {
> > +     case UNWIND_RULE_RETAIN:
> > +             if (!regs)
> > +                     return -EINVAL;
> > +             ra = regs->regs[30];
>
> Likewise: Topmost frame check not required to access arbitrary registers
> (including RA/LR)?  Furthermore, provided don't have a thinko, LR may
> only be in LR in the topmost frame.  In any other frame it must have
> been saved.  Otherwise there would be an endless return loop.
>
> > +             source = KUNWIND_SOURCE_REGS_LR;
> > +             break;
> > +     /* UNWIND_USER_RULE_CFA_OFFSET not implemented on purpose */
> > +     case UNWIND_RULE_CFA_OFFSET_DEREF:
> > +             ra = cfa + frame.ra.offset;
> > +             break;
> > +     case UNWIND_RULE_REG_OFFSET:
> > +     case UNWIND_RULE_REG_OFFSET_DEREF:
> > +             if (!regs)
>
>                 if (!regs || frame.cfa.regnum > 30)
>
> > +                     return -EINVAL;
> > +             ra = regs->regs[frame.cfa.regnum];
>
> Likewise: Topmost frame check not required to access arbitrary registers?
>
> > +             ra += frame.ra.offset;
> > +             break;
> > +     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 */
> > +     case UNWIND_RULE_CFA_OFFSET_DEREF:
> > +             fp = cfa + frame.fp.offset;
> > +             break;
> > +     case UNWIND_RULE_REG_OFFSET:
> > +     case UNWIND_RULE_REG_OFFSET_DEREF:
> > +             if (!regs)
>
>                 if (!regs || frame.cfa.regnum > 30)
>
> > +                     return -EINVAL;
> > +             fp = regs->regs[frame.fp.regnum];
>
> Likewise: Topmost frame check not required to access arbitrary registers?
>
> > +             fp += frame.fp.offset;
> > +             break;
> > +     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;
> > +
> > +     if (frame.ra.rule & UNWIND_RULE_DEREF &&
> > +         get_consume_word(&state->common, &ra))
> > +             return -EINVAL;
> > +
> > +     state->common.pc = ra;
> > +     state->common.sp = cfa;
> > +     state->common.fp = fp;
> > +
> > +     state->source = source;
> > +
> > +     return 0;
> > +}
> Thanks and regards,
> Jens
> --
> Jens Remus
> Linux on Z Development (D3303)
> jremus@xxxxxxxxxx / jremus@xxxxxxxxxxxxx
>
> 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/
>

Thanks,
Dylan