Re: [PATCH v14 00/19] tracing: fprobe: function_graph: Multi-function graph and fprobe on fgraph

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri, 13 Sep 2024 21:45:15 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Fri, 13 Sep 2024 17:59:35 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > > 
> > > Taking failing output from the test:
> > > 
> > > > > > kprobe_multi_testmod_check:FAIL:kretprobe_test3_result unexpected kretprobe_test3_result: actual 0 != expected 1
> > > 
> > > kretprobe_test3_result is a sort of identifier for a test condition,
> > > you can find a corresponding line in user space .c file grepping for
> > > that:
> > > 
> > > ASSERT_EQ(skel->bss->kretprobe_testmod_test3_result, 1,
> > > "kretprobe_test3_result");
> > > 
> > > Most probably the problem is in:
> > > 
> > > __u64 addr = bpf_get_func_ip(ctx);
> > 
> > Yeah, and as I replyed to another thread, the problem is
> > that the ftrace entry_ip is not symbol ip.
> > 
> > We have ftrace_call_adjust() arch function for reverse
> > direction (symbol ip to ftrace entry ip) but what we need
> > here is the reverse translate function (ftrace entry to symbol)
> > 
> > The easiest way is to use kallsyms to find it, but this is
> > a bit costful (but it just increase bsearch in several levels).
> > Other possible options are
> > 
> >  - Change bpf_kprobe_multi_addrs_cmp() to accept a range
> >    of address. [sym_addr, sym_addr + offset) returns true,
> >    bpf_kprobe_multi_cookie() can find the entry address.
> >    The offset depends on arch, but 16 is enough.
> 
> Oops. no, this bpf_kprobe_multi_cookie() is used only for storing
> test data. Not a general problem solving. (I saw kprobe_multi_check())
> 
> So solving problem is much costly, we need to put more arch-
> dependent in bpf_trace, and make sure it does reverse translation
> of ftrace_call_adjust(). (this means if you expand arch support,
> you need to add such implementation)

OK, can you try this one?


From 81bc599911507215aa9faa1077a601880cbd654a Mon Sep 17 00:00:00 2001
From: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
Date: Fri, 13 Sep 2024 21:43:46 +0900
Subject: [PATCH] bpf: Add get_entry_ip() for arm64

Add get_entry_ip() implementation for arm64. This is based on
the information in ftrace_call_adjust() for arm64.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/bpf_trace.c | 64 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index deb629f4a510..b0cf6e5b8965 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1066,6 +1066,70 @@ static unsigned long get_entry_ip(unsigned long fentry_ip)
 		fentry_ip -= ENDBR_INSN_SIZE;
 	return fentry_ip;
 }
+#elif defined (CONFIG_ARM64)
+#include <asm/insn.h>
+
+static unsigned long get_entry_ip(unsigned long fentry_ip)
+{
+	u32 insn;
+
+	/*
+	 * When using patchable-function-entry without pre-function NOPS, ftrace
+	 * entry is the address of the first NOP after the function entry point.
+	 *
+	 * The compiler has either generated:
+	 *
+	 * func+00:	func:	NOP		// To be patched to MOV X9, LR
+	 * func+04:		NOP		// To be patched to BL <caller>
+	 *
+	 * Or:
+	 *
+	 * func-04:		BTI	C
+	 * func+00:	func:	NOP		// To be patched to MOV X9, LR
+	 * func+04:		NOP		// To be patched to BL <caller>
+	 *
+	 * The fentry_ip is the address of `BL <caller>` which is at `func + 4`
+	 * bytes in either case.
+	 */
+	if (!IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
+		return fentry_ip - AARCH64_INSN_SIZE;
+
+	/*
+	 * When using patchable-function-entry with pre-function NOPs, BTI is
+	 * a bit different.
+	 *
+	 * func+00:	func:	NOP		// To be patched to MOV X9, LR
+	 * func+04:		NOP		// To be patched to BL <caller>
+	 *
+	 * Or:
+	 *
+	 * func+00:	func:	BTI	C
+	 * func+04:		NOP		// To be patched to MOV X9, LR
+	 * func+08:		NOP		// To be patched to BL <caller>
+	 *
+	 * The fentry_ip is the address of `BL <caller>` which is at either
+	 * `func + 4` or `func + 8` depends on whether there is a BTI.
+	 */
+
+	/* If there is no BTI, the func address should be one instruction before. */
+	if (!IS_ENABLED(CONFIG_ARM64_BTI_KERNEL))
+		return fentry_ip - AARCH64_INSN_SIZE;
+
+	/* We want to be extra safe in case entry ip is on the page edge,
+	 * but otherwise we need to avoid get_kernel_nofault()'s overhead.
+	 */
+	if ((fentry_ip & ~PAGE_MASK) < AARCH64_INSN_SIZE * 2) {
+		if (get_kernel_nofault(insn, (u32 *)(fentry_ip - AARCH64_INSN_SIZE * 2)))
+			return fentry_ip - AARCH64_INSN_SIZE;
+	} else {
+		insn = *(u32 *)(fentry_ip - AARCH64_INSN_SIZE * 2);
+	}
+
+	if (aarch64_insn_is_bti(le32_to_cpu((__le32)insn)))
+		return fentry_ip - AARCH64_INSN_SIZE * 2;
+
+	return fentry_ip - AARCH64_INSN_SIZE;
+}
 #else
 #define get_entry_ip(fentry_ip) fentry_ip
 #endif
-- 
2.34.1



-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>