Re: [PATCH -tip v8 11/13] x86/unwind: Recover kretprobe trampoline entry

From: Peter Zijlstra <peterz@infradead.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Ingo Molnar <mingo@kernel.org>, X86 ML <x86@kernel.org>,
	Daniel Xu <dxu@dxuuu.xyz>,
	linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
	kuba@kernel.org, mingo@redhat.com, ast@kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Borislav Petkov <bp@alien8.de>,
	kernel-team@fb.com, yhs@fb.com, linux-ia64@vger.kernel.org,
	Abhishek Sagar <sagar.abhishek@gmail.com>,
	Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	wuqiang.matt@bytedance.com
Subject: Re: [PATCH -tip v8 11/13] x86/unwind: Recover kretprobe trampoline entry
Date: Wed, 7 Jul 2021 10:20:41 +0200	[thread overview]
Message-ID: <YOVj2VoyrcOvJfEB@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20210706111136.7c5e9843@oasis.local.home>

On Tue, Jul 06, 2021 at 11:11:36AM -0400, Steven Rostedt wrote:
> On Tue, 6 Jul 2021 09:55:03 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > But I'm not so sure how ftrace treat it. It seems that the return_to_handler()
> > > doesn't care such case. (anyway, return_to_handler() does not return but jump
> > > to the original call-site, in that case, the information will be lost.)  
> > 
> > I find it bothersome (OCD, sorry :-) that both return trampolines behave
> > differently. Doubly so because I know people (Steve in particular) have
> > been talking about unifying them.
> 
> They were developed separately, and designed differently with different
> goals in mind. Yes, I want to unify them, but trying to get the
> different goals together, compounded by the fact that almost every arch
> also implemented them differently (in which case, we need to find a way
> to do it one arch at a time), makes the process extremely frustrating.

Yeah.. that's going to be somewhat painful.

> > Steve, can you clarify the ftrace side here? Afaict return_to_handler()
> > is similarly affected.
> 
> I'm not exactly sure what the issue is. As Masami stated, kretprobe
> uses a ret to return to the calling function, but ftrace uses a jmp.

I'll have to re-read the ftrace bits, but from the top of my head you
cannot do an indirect jump and preserve all registers at the same time,
so a return stub must use jump from stack aka. ret.

> kretprobe return tracing is more complex than the function graph return
> tracing is (which is one of the issues I need to overcome to unify
> them),

I'm not sure it is. IIRC the biggest pain point with kretprobe is that
'silly' property that the kretprobe_instance are not the same between
kretprobes. Luckily, that's not actually used anywhere, so we can simply
rip that out.

That should also help Matt make the whole freelist thing faster, because
now the kretprobe instances are global.

> and when the function graph return trampoline was created, it
> did things as simple as possible (and before ORC existed).
> 
> Is this something to worry about now, or should we look to fix his in
> the unifying process?

There seems to be a lot of kretprobe activity now; so I figured we ought
to at least consider where we want to go so we don't make it harder
still.