Re: [PATCH] arm64: stacktrace: skip reporting LR at exception boundaries

[Mark Rutland <mark.rutland@arm.com>]

On Mon, Dec 09, 2024 at 11:03:51AM +0000, Mark Rutland wrote:
> Recently the arm64 stacktrace code was modified to report the LR at
> exception boundaries, which interacts poorly with fgraph tracing. It is
> possible for the LR to contain the start address of return_to_handler()
> even when the LR is not live, and in such cases attempts to recover the
> return address via ftrace_graph_ret_addr() may fail, triggering a
> WARN_ON_ONCE() in kunwind_recover_return_address() and aborting the
> unwind. This has resulted in test failures and unexpected warnings, as
> reported by Aishwarya and Kent.

To clarify, the issue reported by Kent at:

  http://lore.kernel.org/linux-arm-kernel/zbwbgkuvvciezpmigcp6gaahfxwm7cwhpzus7gtbfnbzsjb2n3@kfbdppbd74o4

... seems to be a distinct issue, and I has misunderstood the report
while writing up this commit message.

Regardless of that, I think this patch is still justified, as it does
address the issue that Aishwarya reported.

Mark.

> 
> Handling unreliable LR values in these cases is likely to require some
> larger rework, so for the moment avoid this problem by restoring the old
> behaviour of skipping the LR at exception boundaries, as we did prior to
> commit:
> 
>   c2c6b27b5aa14fa2 ("arm64: stacktrace: unwind exception boundaries")
> 
> This commit is effectively a partial revert, keeping the structures and
> logic to explicitly identify exception boundaries while still skipping
> reporting of the LR. The logic to explicitly identify exception
> boundaries is still useful for general robustness and as a building
> block for future support for reliably stacktracing.
> 
> Fixes: c2c6b27b5aa14fa2 ("arm64: stacktrace: unwind exception boundaries")
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Reported-by: Aishwarya TCV <aishwarya.tcv@arm.com>
> Reported-by: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/stacktrace.c | 24 ++----------------------
>  1 file changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index caef85462acb6..4a08ad8158380 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -26,7 +26,6 @@ enum kunwind_source {
>  	KUNWIND_SOURCE_CALLER,
>  	KUNWIND_SOURCE_TASK,
>  	KUNWIND_SOURCE_REGS_PC,
> -	KUNWIND_SOURCE_REGS_LR,
>  };
>  
>  union unwind_flags {
> @@ -178,23 +177,8 @@ int kunwind_next_regs_pc(struct kunwind_state *state)
>  	state->regs = regs;
>  	state->common.pc = regs->pc;
>  	state->common.fp = regs->regs[29];
> -	state->source = KUNWIND_SOURCE_REGS_PC;
> -	return 0;
> -}
> -
> -static __always_inline int
> -kunwind_next_regs_lr(struct kunwind_state *state)
> -{
> -	/*
> -	 * The stack for the regs was consumed by kunwind_next_regs_pc(), so we
> -	 * cannot consume that again here, but we know the regs are safe to
> -	 * access.
> -	 */
> -	state->common.pc = state->regs->regs[30];
> -	state->common.fp = state->regs->regs[29];
>  	state->regs = NULL;
> -	state->source = KUNWIND_SOURCE_REGS_LR;
> -
> +	state->source = KUNWIND_SOURCE_REGS_PC;
>  	return 0;
>  }
>  
> @@ -274,11 +258,8 @@ kunwind_next(struct kunwind_state *state)
>  	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 = kunwind_next_regs_lr(state);
> +		err = kunwind_next_frame_record(state);
>  		break;
>  	default:
>  		err = -EINVAL;
> @@ -436,7 +417,6 @@ static const char *state_source_string(const struct kunwind_state *state)
>  	case KUNWIND_SOURCE_CALLER:	return "C";
>  	case KUNWIND_SOURCE_TASK:	return "T";
>  	case KUNWIND_SOURCE_REGS_PC:	return "P";
> -	case KUNWIND_SOURCE_REGS_LR:	return "L";
>  	default:			return "U";
>  	}
>  }
> -- 
> 2.30.2
>