Re: [PATCH RFC] ftrace: Show all functions with addresses in available_filter_functions_addrs

[Jiri Olsa]

On Fri, Jun 09, 2023 at 11:29:59AM -0700, Andrii Nakryiko wrote:
> On Fri, Jun 9, 2023 at 9:44 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
> >
> > On Fri, Jun 09, 2023 at 09:24:10AM +0100, Mark Rutland wrote:
> > > On Thu, Jun 08, 2023 at 04:55:40PM -0700, Andrii Nakryiko wrote:
> > > > On Thu, Jun 8, 2023 at 4:27 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> > > > > On Thu, 8 Jun 2023 15:43:03 -0700 Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> > > > > > On Thu, Jun 8, 2023 at 2:26 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> > >
> > > > There are BPF tools that allow user to specify regex/glob of kernel
> > > > functions to attach to. This regex/glob is checked against
> > > > available_filter_functions to check which functions are traceable. All
> > > > good. But then also it's important to have corresponding memory
> > > > addresses for selected functions (for many reasons, e.g., to have
> > > > non-ambiguous and fast attachment by address instead of by name, or
> > > > for some post-processing based on captured IP addresses, etc). And
> > > > that means that now we need to also parse /proc/kallsyms and
> > > > cross-join it with data fetched from available_filter_functions.
> > > >
> > > > All this is unnecessary if avalable_filter_functions would just
> > > > provide function address in the first place. It's a huge
> > > > simplification. And saves memory and CPU.
> > >
> > > Do you need the address of the function entry-point or the address of the
> > > patch-site within the function? Those can differ, and the rec->ip address won't
> > > necessarily equal the address in /proc/kallsyms, so the pointer in
> > > /proc/kallsyms won't (always) match the address we could print for the ftrace site.
> > >
> > > On arm64, today we can have offsets of +0, +4, and +8, and within a single
> > > kernel image different functions can have different offsets. I suspect in
> > > future that we may have more potential offsets (e.g. due to changes for HW/SW
> > > CFI).
> >
> > so we need that for kprobe_multi bpf link, which is based on fprobe,
> > and that uses ftrace_set_filter_ips to setup the ftrace_ops filter
> >
> > and ftrace_set_filter_ips works fine with ip address being the address
> > of the patched instruction (it's matched in ftrace_location)
> >
> > but right, I did not realize this.. it might cause confusion if people
> > don't know it's patch-side addresses..  not sure if there's easy way to
> > get real function address out of rec->ip, but it will also get more
> > complicated on x86 when IBT is enabled, will check
> 
> ok, sorry, I'm confused. Two questions:
> 
> 1. when attaching kprobe_multi, does bpf() syscall expect function
> address or (func+offset_of_mcount) address? I hope it's the former,
> just function's address?

it can be both, the fprobe/ftrace filter setup will take care of looking
up and translating the provided address to the patch-side address

> 
> 2. If rec->ip is not function's address, can we somehow adjust the
> value to be a function address before printing it?

that's tricky because on x86 with IBT we would need to read the first
instruction and check if it's endbr to get the real address, like we
do in get_entry_ip:

	#ifdef CONFIG_X86_KERNEL_IBT
	static unsigned long get_entry_ip(unsigned long fentry_ip)
	{
		u32 instr;

		/* Being extra safe in here in case entry ip is on the page-edge. */
		if (get_kernel_nofault(instr, (u32 *) fentry_ip - 1))
			return fentry_ip;
		if (is_endbr(instr))
			fentry_ip -= ENDBR_INSN_SIZE;
		return fentry_ip;
	}
	#else
	#define get_entry_ip(fentry_ip) fentry_ip
	#endif


I'm not familiar with arm implementation, so I'm not sure if there's
a way to do this

but in any case Steven wants to keep the patch-side address:
  https://lore.kernel.org/bpf/CAEf4BzbgsLOoLKyscq6S95QeehVoAzOnQ=xmsFz8dfEUAnhObw@xxxxxxxxxxxxxx/T/#m19a97bbc8f8236ad869c9f8ad0cc7dbce0722d92

jirka

> 
> In short, I think it's confusing to have addresses with +0 or +4 or +8
> offsets. It would be great if we can just keep it +0 at the interface
> level (when attach and in available_filter_functions_addrs).
> 
> >
> > or we could just use patch-side addresses and reflect that in the file's
> > name like 'available_filter_functions_patch_addrs' .. it's already long
> > name ;-)
> >
> > jirka