Re: [PATCH bpf-next 0/5] bpf: Fixes for CONFIG_X86_KERNEL_IBT

[Jiri Olsa <olsajiri@gmail.com>]

On Sun, Jul 31, 2022 at 11:08:16PM +0200, Jiri Olsa wrote:
> On Fri, Jul 29, 2022 at 03:18:54PM -0700, Andrii Nakryiko wrote:
> > On Sun, Jul 24, 2022 at 2:21 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > hi,
> > > Martynas reported bpf_get_func_ip returning +4 address when
> > > CONFIG_X86_KERNEL_IBT option is enabled and I found there are
> > > some failing bpf tests when this option is enabled.
> > >
> > > The CONFIG_X86_KERNEL_IBT option adds endbr instruction at the
> > > function entry, so the idea is to 'fix' entry ip for kprobe_multi
> > > and trampoline probes, because they are placed on the function
> > > entry.
> > >
> > > For kprobes I only fixed the bpf test program to adjust ip based
> > > on CONFIG_X86_KERNEL_IBT option. I'm not sure what the right fix
> > > should be in here, because I think user should be aware where the
> > 
> > user can't be aware of this when using multi-kprobe attach by symbolic
> > name of the function. So I think bpf_get_func_ip() at least in that
> > case should be compensating for KERNEL_IBT.
> 
> sorry I said kprobes, but that does not include kprobe multi link,
> I meant what you call general kprobe below
> 
> I do the adjustment for kprobe multi version of bpf_get_func_ip,
> so that should be fine
> 
> > 
> > BTW, given in general kprobe can be placed in them middle of the
> > function, should bpf_get_func_ip() return zero or something for such
> > cases instead of wrong value somewhere in the middle of kprobe? If
> > user cares about current IP, they can get it with PT_REGS_IP(ctx),
> > right?
> 
> true.. we could add flag to 'struct kprobe' to indicate it's placed
> on function's entry and check on endbr instruction for IBT config,
> and return 0 for anything else

Masami,
we'd like to be able to tell if the kprobe is placed on the function
entry, so we could return its address in the get_func_ip helper in
such case

would a new flag for this be acceptable for struct kprobe?

I squashed the kprobe change together with our usage, because it's
shows the usage nicely and it's small diff ;-)

thanks,
jirka


---
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 55041d2f884d..a0b92be98984 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -103,6 +103,7 @@ struct kprobe {
 				   * this flag is only for optimized_kprobe.
 				   */
 #define KPROBE_FLAG_FTRACE	8 /* probe is using ftrace */
+#define KPROBE_FLAG_ON_FUNC_ENTRY	16 /* probe is on the function entry */
 
 /* Has this kprobe gone ? */
 static inline bool kprobe_gone(struct kprobe *p)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index f214f8c088ed..a6b1b5c49d92 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1605,9 +1605,10 @@ int register_kprobe(struct kprobe *p)
 	struct kprobe *old_p;
 	struct module *probed_mod;
 	kprobe_opcode_t *addr;
+	bool on_func_entry;
 
 	/* Adjust probe address from symbol */
-	addr = kprobe_addr(p);
+	addr = _kprobe_addr(p->addr, p->symbol_name, p->offset, &on_func_entry);
 	if (IS_ERR(addr))
 		return PTR_ERR(addr);
 	p->addr = addr;
@@ -1627,6 +1628,9 @@ int register_kprobe(struct kprobe *p)
 
 	mutex_lock(&kprobe_mutex);
 
+	if (on_func_entry)
+		p->flags |= KPROBE_FLAG_ON_FUNC_ENTRY;
+
 	old_p = get_kprobe(p->addr);
 	if (old_p) {
 		/* Since this may unoptimize 'old_p', locking 'text_mutex'. */
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index bcada91b0b3b..f80c642d7491 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1029,8 +1029,17 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_tracing = {
 BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
 {
 	struct kprobe *kp = kprobe_running();
+	uintptr_t addr;
 
-	return kp ? (uintptr_t)kp->addr : 0;
+	if (!kp || !(kp->flags & KPROBE_FLAG_ON_FUNC_ENTRY))
+		return 0;
+
+	addr = (uintptr_t)kp->addr;
+#ifdef CONFIG_X86_KERNEL_IBT
+	if (is_endbr(*((u32 *) entry_ip - 1)))
+		addr -= ENDBR_INSN_SIZE;
+#endif
+	return addr;
 }
 
 static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = {
diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
index a587aeca5ae0..6db70757bc8b 100644
--- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c
+++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
@@ -69,7 +69,7 @@ int test6(struct pt_regs *ctx)
 {
 	__u64 addr = bpf_get_func_ip(ctx);
 
-	test6_result = (const void *) addr == &bpf_fentry_test6 + 5;
+	test6_result = (const void *) addr == 0;
 	return 0;
 }
 
@@ -79,6 +79,6 @@ int test7(struct pt_regs *ctx)
 {
 	__u64 addr = bpf_get_func_ip(ctx);
 
-	test7_result = (const void *) addr == &bpf_fentry_test7 + 5;
+	test7_result = (const void *) addr == 0;
 	return 0;
 }