On Wed, 10 Mar 2021 12:31:13 -0600
Josh Poimboeuf <jpoim...@redhat.com> wrote:
> 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.
Yes, anyway dummy kretprobe_find_ret_addr() and kretprobe_trampoline_addr()
should be defined !CONFIG_KRETPROBES case.
>
> > 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?
As I commented above, until kretprobe_trampoline writes back the real
address to the stack, sp value is there (which has been pushed by the
'pushq %rsp' at the entry of kretprobe_trampoline.)
".type kretprobe_trampoline, @function\n"
"kretprobe_trampoline:\n"
/* We don't bother saving the ss register */
" pushq %rsp\n" // THIS
" pushfq\n"
Thus, from inside the kretprobe handler, like ftrace, you'll see
the sp value instead of the real return address.
> > + 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);
OK, but,
> and without the conditional.
As I said, it is not possible because "state->ip == sp" check depends on
ORC unwinder.
> > 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'.
Hmm, actually current implementation on x86 mimics the behevior of
the int3 exception (which many architectures still do).
Previously the kretprobe_trampoline is a place holder like this.
"kretprobe_trampoline:\n"
" nop\n"
And arch_init_kprobes() puts a kprobe (int3) there.
So in that case regs->ip should be kretprobe_trampoline.
User handler (usually architecutre independent) finds the
correct_ret_addr in kretprobe_instance.ret_addr field.
> 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.
At that moment, the real return address is not identified. So we can not
put it.
>
> 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.
Thank you,
--
Masami Hiramatsu <mhira...@kernel.org>
