Re: [RFC PATCH -tip 3/3] x86/kprobes,orc: Unwind optprobe trampoline correctly

[Masami Hiramatsu]

On Thu, 1 Apr 2021 10:44:52 +0900
Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:

> On Wed, 31 Mar 2021 10:57:36 -0500
> Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> 
> > On Wed, Mar 31, 2021 at 02:44:56PM +0900, Masami Hiramatsu wrote:
> > > +#ifdef CONFIG_UNWINDER_ORC
> > > +unsigned long recover_optprobe_trampoline(unsigned long addr, unsigned long *sp)
> > > +{
> > > +	unsigned long offset, entry, probe_addr;
> > > +	struct optimized_kprobe *op;
> > > +	struct orc_entry *orc;
> > > +
> > > +	entry = find_kprobe_optinsn_slot_entry(addr);
> > > +	if (!entry)
> > > +		return addr;
> > > +
> > > +	offset = addr - entry;
> > > +
> > > +	/* Decode arg1 and get the optprobe */
> > > +	op = (void *)extract_set_arg1((void *)(entry + TMPL_MOVE_IDX));
> > > +	if (!op)
> > > +		return addr;
> > > +
> > > +	probe_addr = (unsigned long)op->kp.addr;
> > > +
> > > +	if (offset < TMPL_END_IDX) {
> > > +		orc = orc_find((unsigned long)optprobe_template_func + offset);
> > > +		if (!orc || orc->sp_reg != ORC_REG_SP)
> > > +			return addr;
> > > +		/*
> > > +		 * Since optprobe trampoline doesn't push caller on the stack,
> > > +		 * need to decrement 1 stack entry size
> > > +		 */
> > > +		*sp += orc->sp_offset - sizeof(long);
> > > +		return probe_addr;
> > > +	} else {
> > > +		return probe_addr + offset - TMPL_END_IDX;
> > > +	}
> > > +}
> > > +#endif
> > 
> > Hm, I'd like to avoid intertwining kprobes and ORC like this.
> > 
> > ORC unwinds other generated code by assuming the generated code uses a
> > frame pointer.  Could we do that here?
> 
> No, because the optprobe is not a function call. I considered to make
> it call, but since it has to execute copied instructions directly on
> the trampoline code (without changing stack frame) it is not possible.
> 
> > With CONFIG_FRAME_POINTER, unwinding works because SAVE_REGS_STRING has
> > ENCODE_FRAME_POINTER, but that's not going to work for ORC.
> 
> Even in that case, the problem is that any interrupt can happen
> before doing ENCODE_FRAME_POINTER. I think this ENCODE_FRAME_POINTER
> in the SAVE_REGS_STRING is for probing right before the target
> function setup a frame pointer.
> 
> > Instead of these patches, can we 'push %rbp; mov %rsp, %rbp' at the
> > beginning of the template and 'pop %rbp' at the end?
> 
> No, since the trampoline code is not called, it is jumped into.
> This means there is no "return address" in the stack. If we setup
> the frame, there is no return address, thus it might stop there.
> (Moreover, optprobe can copy multiple instructins on trampoline
> buffer, since relative jump consumes 5bytes. where is the "return address"?)
> 
> > 
> > I guess SAVE_REGS_STRING would need to be smart enough to push the
> > original saved version of %rbp.  Of course then that breaks the
> > kretprobe_trampoline() usage, so it may need to be a separate macro.
> > 
> > [ Or make the same change to kretprobe_trampoline().  Then the other
> >   patch set wouldn't be needed either ;-) ]
> 
> Hmm, I don't think it is a good idea which making such change on the
> optimized (hot) path only for the stack tracing. Moreover, that maybe
> not transparent with the stack made by int3.
> 
> > Of course the downside is, when you get an interrupt during the frame
> > pointer setup, unwinding is broken.  But I think that's acceptable for
> > generated code.  We've lived with that limitation for all code, with
> > CONFIG_FRAME_POINTER, for many years.
> 
> But above code can fix such issue too. To fix a corner case, non-generic
> code may be required, even it is not so simple.

Hmm, I would like to confirm your policy on ORC unwinder. If it doesn't
care the stacktrace from the interrupt handler, I think your suggestion
is OK. But in that case, from a developer viewpoint, I need to recommend
users to configure CONFIG_UNWIND_FRAME=y when CONFIG_KPROBES=y.

> > Eventually we may want to have a way to register generated code (and the
> > ORC for it).

I see, but the generated code usually does not have a generic way to
handle it. E.g. bpf has a solid entry point, but kretprobe trampoline's
entry point is any "RET", optprobe trampoline's entry point is a jump
which is also generated (patched) ...

Thank you,

-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>