Re: [RFC PATCH v3 0/9] fprobe: Introduce fprobe function entry/exit probe

From: Jiri Olsa <jolsa@redhat.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	lkml <linux-kernel@vger.kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	"Naveen N . Rao" <naveen.n.rao@linux.ibm.com>,
	Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>,
	"David S . Miller" <davem@davemloft.net>
Subject: Re: [RFC PATCH v3 0/9] fprobe: Introduce fprobe function entry/exit probe
Date: Mon, 24 Jan 2022 23:23:20 +0100	[thread overview]
Message-ID: <Ye8m2CpVI8VOiMTH@krava> (raw)
In-Reply-To: <CAEf4BzbrVBXDJA4qbCgudiiLGtHNyUQAOuE=AUwfxzMrF=Wr=w@mail.gmail.com>

On Mon, Jan 24, 2022 at 12:22:10PM -0800, Andrii Nakryiko wrote:

SNIP

> > > > > > > > (This testing patch is just for confirming the rethook is correctly
> > > > > > > >  implemented.)
> > > > > > > >
> > > > > > > > BTW, on the x86, ftrace (with fentry) location address is same as
> > > > > > > > symbol address. But on other archs, it will be different (e.g. arm64
> > > > > > > > will need 2 instructions to save link-register and call ftrace, the
> > > > > > > > 2nd instruction will be the ftrace location.)
> > > > > > > > Does libbpf correctly handle it?
> > > >
> > > > hm, I'm probably missing something, but should this be handled by arm
> > > > specific kernel code? user passes whatever is found in kallsyms, right?
> > >
> > > In x86, fentry nop is always placed at the first instruction of the function,
> > > but the other arches couldn't do that if they use LR (link register) for
> > > storing return address instead of stack. E.g. arm64 saves lr and call the
> > > ftrace. Then ftrace location address of a function is not the symbol address.
> > >
> > > Anyway, I updated fprobe to handle those cases. I also found some issues
> > > on rethook, so let me update the series again.
> >
> > great, I reworked the bpf fprobe link change and need to add the
> > symbols attachment support, so you don't need to include it in
> > new version.. I'll rebase it and send on top of your patchset
> 
> Using just addresses (IPs) for retsnoop and bpftrace is fine because
> such generic tools are already parsing kallsyms and probably building
> some lookup table. But in general, having IP-based attachment is a
> regression from current perf_event_open-based kprobe, where user is
> expected to pass symbolic function name. Using IPs has an advantage of
> being unambiguous (e.g., when same static function name in kernel
> belongs to multiple actual functions), so there is that. But I was
> also wondering wouldn't kernel need to do symbol to IP resolution
> anyways just to check that we are attaching to function entry?

ftrace does its own check for address to attach, it keeps record
for every attachable address.. so less work for us ;-)

> 
> I'll wait for your patch set to see how did you go about it in a new revision.

I agree we should have the support to use symbols as well, I'll add it

jirka