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@xxxxxxxxxx> 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;
}
