Re: [PATCH 7/9] x86/unwind/orc: Fall back to using frame pointers for generated code

[Alexei Starovoitov]

On Thu, Jun 13, 2019 at 08:21:04AM -0500, Josh Poimboeuf wrote:
> The ORC unwinder can't unwind through BPF JIT generated code because
> there are no ORC entries associated with the code.
> 
> If an ORC entry isn't available, try to fall back to frame pointers.  If
> BPF and other generated code always do frame pointer setup (even with
> CONFIG_FRAME_POINTERS=n) then this will allow ORC to unwind through most
> generated code despite there being no corresponding ORC entries.
> 
> Fixes: d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
> Reported-by: Song Liu <songliubraving@xxxxxx>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
>  arch/x86/kernel/unwind_orc.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index 33b66b5c5aec..72b997eaa1fc 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -82,9 +82,9 @@ static struct orc_entry *orc_find(unsigned long ip);
>   * But they are copies of the ftrace entries that are static and
>   * defined in ftrace_*.S, which do have orc entries.
>   *
> - * If the undwinder comes across a ftrace trampoline, then find the
> + * If the unwinder comes across a ftrace trampoline, then find the
>   * ftrace function that was used to create it, and use that ftrace
> - * function's orc entrie, as the placement of the return code in
> + * function's orc entry, as the placement of the return code in
>   * the stack will be identical.
>   */
>  static struct orc_entry *orc_ftrace_find(unsigned long ip)
> @@ -128,6 +128,16 @@ static struct orc_entry null_orc_entry = {
>  	.type = ORC_TYPE_CALL
>  };
>  
> +/* Fake frame pointer entry -- used as a fallback for generated code */
> +static struct orc_entry orc_fp_entry = {
> +	.type		= ORC_TYPE_CALL,
> +	.sp_reg		= ORC_REG_BP,
> +	.sp_offset	= 16,
> +	.bp_reg		= ORC_REG_PREV_SP,
> +	.bp_offset	= -16,
> +	.end		= 0,
> +};
> +
>  static struct orc_entry *orc_find(unsigned long ip)
>  {
>  	static struct orc_entry *orc;
> @@ -392,8 +402,16 @@ bool unwind_next_frame(struct unwind_state *state)
>  	 * calls and calls to noreturn functions.
>  	 */
>  	orc = orc_find(state->signal ? state->ip : state->ip - 1);
> -	if (!orc)
> -		goto err;
> +	if (!orc) {
> +		/*
> +		 * As a fallback, try to assume this code uses a frame pointer.
> +		 * This is useful for generated code, like BPF, which ORC
> +		 * doesn't know about.  This is just a guess, so the rest of
> +		 * the unwind is no longer considered reliable.
> +		 */
> +		orc = &orc_fp_entry;
> +		state->error = true;

That seems fragile.
Can't we populate orc_unwind tables after JIT ?