Re: [PATCH -tip 0/5] kprobes: Fix stacktrace in kretprobes

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Thu, Mar 11, 2021 at 12:55:09AM +0900, Masami Hiramatsu wrote:
> +#ifdef CONFIG_KRETPROBES
> +static unsigned long orc_kretprobe_correct_ip(struct unwind_state *state)
> +{
> +	return kretprobe_find_ret_addr(
> +			(unsigned long)kretprobe_trampoline_addr(),
> +			state->task, &state->kr_iter);
> +}
> +
> +static bool is_kretprobe_trampoline_address(unsigned long ip)
> +{
> +	return ip == (unsigned long)kretprobe_trampoline_addr();
> +}
> +#else
> +static unsigned long orc_kretprobe_correct_ip(struct unwind_state *state)
> +{
> +	return state->ip;
> +}
> +
> +static bool is_kretprobe_trampoline_address(unsigned long ip)
> +{
> +	return false;
> +}
> +#endif
> +

Can this code go in a kprobes file?  I'd rather not clutter ORC with it,
and maybe it would be useful for other arches or unwinders.

>  bool unwind_next_frame(struct unwind_state *state)
>  {
>  	unsigned long ip_p, sp, tmp, orig_ip = state->ip, prev_sp = state->sp;
> @@ -536,6 +561,18 @@ bool unwind_next_frame(struct unwind_state *state)
>  
>  		state->ip = ftrace_graph_ret_addr(state->task, &state->graph_idx,
>  						  state->ip, (void *)ip_p);
> +		/*
> +		 * There are special cases when the stack unwinder is called
> +		 * from the kretprobe handler or the interrupt handler which
> +		 * occurs in the kretprobe trampoline code. In those cases,
> +		 * %sp is shown on the stack instead of the return address.
> +		 * Or, when the unwinder find the return address is replaced
> +		 * by kretprobe_trampoline.
> +		 * In those cases, correct address can be found in kretprobe.
> +		 */
> +		if (state->ip == sp ||

Why is the 'state->ip == sp' needed?

> +		    is_kretprobe_trampoline_address(state->ip))
> +			state->ip = orc_kretprobe_correct_ip(state);

This is similar in concept to ftrace_graph_ret_addr(), right?  Would it
be possible to have a similar API?  Like

		state->ip = kretprobe_ret_addr(state->task, &state->kr_iter, state->ip);

and without the conditional.

>  
>  		state->sp = sp;
>  		state->regs = NULL;
> @@ -649,6 +686,12 @@ void __unwind_start(struct unwind_state *state, struct task_struct *task,
>  		state->full_regs = true;
>  		state->signal = true;
>  
> +		/*
> +		 * When the unwinder called with regs from kretprobe handler,
> +		 * the regs->ip starts from kretprobe_trampoline address.
> +		 */
> +		if (is_kretprobe_trampoline_address(state->ip))
> +			state->ip = orc_kretprobe_correct_ip(state);

Shouldn't __kretprobe_trampoline_handler() just set regs->ip to
'correct_ret_addr' before passing the regs to the handler?  I'd think
that would be a less surprising value for regs->ip than
'&kretprobe_trampoline'.

And it would make the unwinder just work automatically when unwinding
from the handler using the regs.

It would also work when unwinding from the handler's stack, if we put an
UNWIND_HINT_REGS after saving the regs.

The only (rare) case it wouldn't work would be unwinding from an
interrupt before regs->ip gets set properly.  In which case we'd still
need the above call to orc_kretprobe_correct_ip() or so.

-- 
Josh