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

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 9 Jul 2025 12:01:14 +0200
Jens Remus <jremus@linux.ibm.com> wrote:
> >  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.

From your latest email, I take it I can ignore the above?

> 
> > +			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.

I believe so.

> 
> > +
> > +	/* 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?

You mean to validate cfa + frame->fp_off?

Isn't cfa the only real variable here? That is, if cfa + frame->ra_off
works, wouldn't the same go for frame->fp_off, as both frame->ra_off
and frame->fp_off are constants set by the architecture, and should be
word aligned.

-- Steve

> 
> > +	if (frame->fp_off && get_user(fp, (unsigned long __user *)(cfa + frame->fp_off)))
> > +		goto done;
> > +