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

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Jiri Olsa <jolsa@redhat.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 14:58:43 -0800	[thread overview]
Message-ID: <CAEf4Bzasj_3EFwW6RvMcV9Z95QUfevUX5eTA5_yWB4Q+KvXuXg@mail.gmail.com> (raw)
In-Reply-To: <Ye8m2CpVI8VOiMTH@krava>

On Mon, Jan 24, 2022 at 2:23 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> 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 ;-)

Oh, makes sense, thanks!

>
> >
> > 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

sounds good, thanks

>
> jirka
>