Messages in this thread Patch in this message | | | Date | Thu, 11 Mar 2021 09:20:18 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH -tip 0/5] kprobes: Fix stacktrace in kretprobes |
| |
On Wed, 10 Mar 2021 12:31:13 -0600 Josh Poimboeuf <jpoimboe@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 <mhiramat@kernel.org>
| |