On Fri, Apr 17, 2026 at 8:45 AM Jens Remus <[email protected]> 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)
> [email protected] / [email protected]
>
> 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
