[PATCH 10/10] arm64: stacktrace: unwind exception boundaries

[Mark Rutland]

On Fri, Oct 11, 2024 at 05:16:23PM +0200, Miroslav Benes wrote:
> Hi,
> 
> > +static __always_inline int
> > +kunwind_next_frame_record(struct kunwind_state *state)
> > +{
> > +	unsigned long fp = state->common.fp;
> > +	struct frame_record *record;
> > +	struct stack_info *info;
> > +	unsigned long new_fp, new_pc;
> > +
> > +	if (fp & 0x7)
> > +		return -EINVAL;
> > +
> > +	info = unwind_find_stack(&state->common, fp, sizeof(*record));
> > +	if (!info)
> > +		return -EINVAL;
> > +
> > +	record = (struct frame_record *)fp;
> > +	new_fp = READ_ONCE(record->fp);
> > +	new_pc = READ_ONCE(record->lr);
> > +
> > +	if (!new_fp && !new_pc)
> > +		return kunwind_next_frame_record_meta(state);
> > +
> > +	unwind_consume_stack(&state->common, info, fp, sizeof(*record));
> > +
> > +	state->common.fp = new_fp;
> > +	state->common.pc = new_pc;
> > +	state->source = KUNWIND_SOURCE_FRAME;
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Unwind from one frame record (A) to the next frame record (B).
> >   *
> > @@ -165,30 +266,27 @@ kunwind_recover_return_address(struct kunwind_state *state)
> >  static __always_inline int
> >  kunwind_next(struct kunwind_state *state)
> >  {
> > -	struct task_struct *tsk = state->task;
> > -	unsigned long fp = state->common.fp;
> >  	int err;
> >  
> >  	state->flags.all = 0;
> >  
> > -	/* Final frame; nothing to unwind */
> > -	if (fp == (unsigned long)&task_pt_regs(tsk)->stackframe)
> > -		return -ENOENT;
> > -
> >  	switch (state->source) {
> >  	case KUNWIND_SOURCE_FRAME:
> >  	case KUNWIND_SOURCE_CALLER:
> >  	case KUNWIND_SOURCE_TASK:
> > +	case KUNWIND_SOURCE_REGS_LR:
> > +		err = kunwind_next_frame_record(state);
> > +		break;
> >  	case KUNWIND_SOURCE_REGS_PC:
> > -		err = unwind_next_frame_record(&state->common);
> > -		if (err)
> > -			return err;
> > -		state->source = KUNWIND_SOURCE_FRAME;
> > +		err = kunwind_next_regs_lr(state);
> 
> the remaining users of unwind_next_frame_record() after this change are in 
> KVM. How does it work there?

The short of it is those are unchanged by this series, and the behaviour
of the KVM stacktrace code is unchanged -- it will continue to not
report exception boundaries.

The KVM hyp code doesn't (currently) use the frame_record_meta records
at exception boundaries, so the KVM stacktrace code won't see those and
only need to unwind regular frame records.

It would be nice to improve that, but IIUC it shouldn't matter for
RELAIBLE_STACKTRACE; all calls to hyp code from the main kernel are
synchronous, and aside from a (fatal) hyp_panic(), executing in a
regular kernel context ensures there's no hyp context to unwind.

> What is the difference?

The kunwind_next_frame_record() function will identify and unwind any
frame_record_meta frames, while unwind_next_frame_record() only handles
regular frame records. Since the KVM hyp code doesn't use
frame_record_meta frames, using unwind_next_frame_record() is
sufficient.

I originally wanted the kunwind code call unwind_next_frame_record() and
then handle any frame_record_meta, but that was painful due to the state
that alters (e.g. the way we track which stack ranges have been
consumed), and duplicating the easrly parts of
unwind_next_frame_record() was simpler overall.

As a general naming convention, the unwind_*() functions are things that
can be shared between the main kernel image and KVM, and the kunwind_*()
functions are specific to the main kernel image.

I'm happy to change the name to more clearly distinguish
kunwind_next_frame_record() from unwind_next_frame_record(), if that
would help?

Mark.