On Sun, Jul 31, 2022 at 2:08 PM Jiri Olsa <olsajiri@xxxxxxxxx> 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 I'd strive for multi-kprobe and kprobe to not have such differences, at least for common function entry (which also means kretprobe, btw) case. Ideally multi-kprobe is just a more efficient (in terms of mass-attachment) version of kprobe with no difference in BPF helpers. So yeah, I totally support your idea of handling that in a helper. > > > > > 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 > > jirka > > > > kprobe is placed, on the other hand we move the kprobe address if > > > its placed on top of endbr instruction. > > > > > > v1 changes: > > > - read previous instruction in kprobe_multi link handler > > > and adjust entry_ip for CONFIG_X86_KERNEL_IBT option > > > - split first patch into 2 separate changes > > > - update changelogs > > > > > > thanks, > > > jirka > > > > > > > > > --- > > > Jiri Olsa (5): > > > ftrace: Keep the resolved addr in kallsyms_callback > > > bpf: Adjust kprobe_multi entry_ip for CONFIG_X86_KERNEL_IBT > > > bpf: Use given function address for trampoline ip arg > > > selftests/bpf: Disable kprobe attach test with offset for CONFIG_X86_KERNEL_IBT > > > selftests/bpf: Fix kprobe get_func_ip tests for CONFIG_X86_KERNEL_IBT > > > > > > arch/x86/net/bpf_jit_comp.c | 9 ++++----- > > > kernel/trace/bpf_trace.c | 4 ++++ > > > kernel/trace/ftrace.c | 3 +-- > > > tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c | 25 ++++++++++++++++++++----- > > > tools/testing/selftests/bpf/progs/get_func_ip_test.c | 7 +++++-- > > > tools/testing/selftests/bpf/progs/kprobe_multi.c | 2 +- > > > 6 files changed, 35 insertions(+), 15 deletions(-)
