Re: [PATCH 2/4] x86/kprobes: Fix frame pointer annotations

[Peter Zijlstra]

On Fri, May 10, 2019 at 01:58:31PM +0900, Masami Hiramatsu wrote:
> On Thu, 9 May 2019 19:14:16 +0200
> Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > > > --- a/arch/x86/kernel/kprobes/core.c
> > > > +++ b/arch/x86/kernel/kprobes/core.c
> > > > @@ -731,29 +731,8 @@ asm(
> > > >  	".global kretprobe_trampoline\n"
> > > >  	".type kretprobe_trampoline, @function\n"
> > > >  	"kretprobe_trampoline:\n"

> > > Here, we need a gap for storing ret-ip, because kretprobe_trampoline is
> > > the address which is returned from the target function. We have no 
> > > "ret-ip" here at this point. So something like
> > > 
> > > +	"push $0\n"	/* This is a gap, will be filled with real return address*/
> > 
> > The trampoline already provides a gap, trampoline_handler() will need to
> > use int3_emulate_push() if it wants to inject something on the return
> > stack.
> 
> I guess you mean the int3 case. This trampoline is used as a return destination.

> When the target function is called, kretprobe interrupts the first instruction,
> and replace the return address with this trampoline. When a "ret" instruction
> is done, it returns to this trampoline. Thus the stack frame start with
> previous context here. As you described above,

I would prefer to change that to inject an extra return address, instead
of replacing it. With the new exception stuff we can actually do that.

So on entry we then go from:

	<previous context>
	RET-IP

to

	<previous context>
	RET-IP
	return-trampoline

So when the function returns, it falls into the trampoline instead.

> > > > +	 * On entry the stack looks like:
> > > > +	 *
> > > > +	 *   2*4(%esp) <previous context>
> > > > +	 *   1*4(%esp) RET-IP
> > > > +	 *   0*4(%esp) func
> 
> From this trampoline call, the stack looks like:
> 
> 	 *   1*4(%esp) <previous context>
> 	 *   0*4(%esp) func
> 
> So we need one more push.

And then the stack looks just right at this point.

> > > > +	"push trampoline_handler\n"
> > > > +	"jmp call_to_exception_trampoline\n"
> > > >  	".size kretprobe_trampoline, .-kretprobe_trampoline\n"
> > > >  );
> > > >  NOKPROBE_SYMBOL(kretprobe_trampoline);