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

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

[Jiri Olsa]

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
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(-)

[PATCH bpf-next 1/5] ftrace: Keep the resolved addr in kallsyms_callback

[Jiri Olsa]

Keeping the resolved 'addr' in kallsyms_callback, instead of taking
ftrace_location value, because we depend on symbol address in the
cookie related code.

With CONFIG_X86_KERNEL_IBT option the ftrace_location value differs
from symbol address, which screwes the symbol address cookies matching.

There are 2 users of this function:
- bpf_kprobe_multi_link_attach
    for which this fix is for

- get_ftrace_locations
    which is used by register_fprobe_syms

    this function needs to get symbols resolved to addresses,
    but does not need 'ftrace location addresses' at this point
    there's another ftrace location translation in the path done
    by ftrace_set_filter_ips call:

     register_fprobe_syms
       addrs = get_ftrace_locations

       register_fprobe_ips(addrs)
         ...
         ftrace_set_filter_ips
           ...
             __ftrace_match_addr
               ip = ftrace_location(ip);
               ...

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/ftrace.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 601ccf1b2f09..3f3c98ee207a 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -8040,8 +8040,7 @@ static int kallsyms_callback(void *data, const char *name,
 	if (args->addrs[idx])
 		return 0;
 
-	addr = ftrace_location(addr);
-	if (!addr)
+	if (!ftrace_location(addr))
 		return 0;
 
 	args->addrs[idx] = addr;
-- 
2.35.3

[PATCH bpf-next 2/5] bpf: Adjust kprobe_multi entry_ip for CONFIG_X86_KERNEL_IBT

[Jiri Olsa]

Martynas reported bpf_get_func_ip returning +4 address when
CONFIG_X86_KERNEL_IBT option is enabled.

When CONFIG_X86_KERNEL_IBT is enabled we'll have endbr instruction
at the function entry, which screws return value of bpf_get_func_ip()
helper that should return the function address.

There's short term workaround [1] for kprobe_multi bpf program made
by Alexei [1], but we need this fixup also for bpf_get_attach_cookie,
that returns cookie based on the entry_ip value.

Moving the fixup in the fprobe handler, so both bpf_get_func_ip
and bpf_get_attach_cookie get expected function address when
CONFIG_X86_KERNEL_IBT option is enabled.

Keeping the resolved 'addr' in kallsyms_callback, instead of taking
ftrace_location value, because we depend on symbol address in the
cookie related code. With CONFIG_X86_KERNEL_IBT option the
ftrace_location value differs from symbol address.

Cc: Peter Zijlstra <peterz@infradead.org>
Reported-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/bpf_trace.c                         | 4 ++++
 tools/testing/selftests/bpf/progs/kprobe_multi.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 68e5cdd24cef..bcada91b0b3b 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2419,6 +2419,10 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
 {
 	struct bpf_kprobe_multi_link *link;
 
+#ifdef CONFIG_X86_KERNEL_IBT
+	if (is_endbr(*((u32 *) entry_ip - 1)))
+		entry_ip -= ENDBR_INSN_SIZE;
+#endif
 	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
 	kprobe_multi_link_prog_run(link, entry_ip, regs);
 }
diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi.c b/tools/testing/selftests/bpf/progs/kprobe_multi.c
index 08f95a8155d1..f165615d0b78 100644
--- a/tools/testing/selftests/bpf/progs/kprobe_multi.c
+++ b/tools/testing/selftests/bpf/progs/kprobe_multi.c
@@ -44,7 +44,7 @@ static void kprobe_multi_check(void *ctx, bool is_return)
 		return;
 
 	__u64 cookie = test_cookie ? bpf_get_attach_cookie(ctx) : 0;
-	__u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
+	__u64 addr = bpf_get_func_ip(ctx);
 
 #define SET(__var, __addr, __cookie) ({			\
 	if (((const void *) addr == __addr) &&		\
-- 
2.35.3

[PATCH bpf-next 3/5] bpf: Use given function address for trampoline ip arg

[Jiri Olsa]

Using function address given at the generation time as the trampoline
ip argument. This way we get directly the function address that we
need, so we don't need to:
  - read the ip from the stack
  - subtract X86_PATCH_SIZE
  - subtract ENDBR_INSN_SIZE if CONFIG_X86_KERNEL_IBT is enabled
    which is not even implemented yet ;-)

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/net/bpf_jit_comp.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 54c7f46c453f..5081ea7b23b6 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1999,13 +1999,14 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end,
 				const struct btf_func_model *m, u32 flags,
 				struct bpf_tramp_links *tlinks,
-				void *orig_call)
+				void *func_addr)
 {
 	int ret, i, nr_args = m->nr_args;
 	int regs_off, ip_off, args_off, stack_size = nr_args * 8, run_ctx_off;
 	struct bpf_tramp_links *fentry = &tlinks[BPF_TRAMP_FENTRY];
 	struct bpf_tramp_links *fexit = &tlinks[BPF_TRAMP_FEXIT];
 	struct bpf_tramp_links *fmod_ret = &tlinks[BPF_TRAMP_MODIFY_RETURN];
+	void *orig_call = func_addr;
 	u8 **branches = NULL;
 	u8 *prog;
 	bool save_ret;
@@ -2078,12 +2079,10 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 
 	if (flags & BPF_TRAMP_F_IP_ARG) {
 		/* Store IP address of the traced function:
-		 * mov rax, QWORD PTR [rbp + 8]
-		 * sub rax, X86_PATCH_SIZE
+		 * mov rax, func_addr
 		 * mov QWORD PTR [rbp - ip_off], rax
 		 */
-		emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
-		EMIT4(0x48, 0x83, 0xe8, X86_PATCH_SIZE);
+		emit_mov_imm64(&prog, BPF_REG_0, (long) func_addr >> 32, (u32) (long) func_addr);
 		emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -ip_off);
 	}
 
-- 
2.35.3

[PATCH bpf-next 4/5] selftests/bpf: Disable kprobe attach test with offset for CONFIG_X86_KERNEL_IBT

[Jiri Olsa]

Attach like 'kprobe/bpf_fentry_test6+0x5' will fail to attach
when CONFIG_X86_KERNEL_IBT option is enabled because of the
endbr instruction at the function entry.

We would need to do manual attach with offset calculation based
on the CONFIG_X86_KERNEL_IBT option, which does not seem worth
the effort to me.

Disabling these test when CONFIG_X86_KERNEL_IBT is enabled.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/prog_tests/get_func_ip_test.c         | 25 +++++++++++++++----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c b/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
index 938dbd4d7c2f..cb0b78fb29df 100644
--- a/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
@@ -2,6 +2,24 @@
 #include <test_progs.h>
 #include "get_func_ip_test.skel.h"
 
+/* assume IBT is enabled when kernel configs are not available */
+#ifdef HAVE_GENHDR
+# include "autoconf.h"
+#else
+#  define CONFIG_X86_KERNEL_IBT 1
+#endif
+
+/* test6 and test7 are x86_64 specific because of the instruction
+ * offset, disabling it for all other archs
+ *
+ * CONFIG_X86_KERNEL_IBT adds endbr instruction at function entry,
+ * so disabling test6 and test7, because the offset is hardcoded
+ * in program section
+ */
+#if !defined(__x86_64__) || defined(CONFIG_X86_KERNEL_IBT)
+#define DISABLE_OFFSET_ATTACH 1
+#endif
+
 void test_get_func_ip_test(void)
 {
 	struct get_func_ip_test *skel = NULL;
@@ -12,10 +30,7 @@ void test_get_func_ip_test(void)
 	if (!ASSERT_OK_PTR(skel, "get_func_ip_test__open"))
 		return;
 
-	/* test6 is x86_64 specifc because of the instruction
-	 * offset, disabling it for all other archs
-	 */
-#ifndef __x86_64__
+#if defined(DISABLE_OFFSET_ATTACH)
 	bpf_program__set_autoload(skel->progs.test6, false);
 	bpf_program__set_autoload(skel->progs.test7, false);
 #endif
@@ -43,7 +58,7 @@ void test_get_func_ip_test(void)
 	ASSERT_EQ(skel->bss->test3_result, 1, "test3_result");
 	ASSERT_EQ(skel->bss->test4_result, 1, "test4_result");
 	ASSERT_EQ(skel->bss->test5_result, 1, "test5_result");
-#ifdef __x86_64__
+#if !defined(DISABLE_OFFSET_ATTACH)
 	ASSERT_EQ(skel->bss->test6_result, 1, "test6_result");
 	ASSERT_EQ(skel->bss->test7_result, 1, "test7_result");
 #endif
-- 
2.35.3

Re: [PATCH bpf-next 4/5] selftests/bpf: Disable kprobe attach test with offset for CONFIG_X86_KERNEL_IBT

[Andrii Nakryiko]

On Sun, Jul 24, 2022 at 2:22 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Attach like 'kprobe/bpf_fentry_test6+0x5' will fail to attach
> when CONFIG_X86_KERNEL_IBT option is enabled because of the
> endbr instruction at the function entry.
>
> We would need to do manual attach with offset calculation based
> on the CONFIG_X86_KERNEL_IBT option, which does not seem worth
> the effort to me.
>
> Disabling these test when CONFIG_X86_KERNEL_IBT is enabled.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../bpf/prog_tests/get_func_ip_test.c         | 25 +++++++++++++++----
>  1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c b/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
> index 938dbd4d7c2f..cb0b78fb29df 100644
> --- a/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
> @@ -2,6 +2,24 @@
>  #include <test_progs.h>
>  #include "get_func_ip_test.skel.h"
>
> +/* assume IBT is enabled when kernel configs are not available */
> +#ifdef HAVE_GENHDR
> +# include "autoconf.h"
> +#else
> +#  define CONFIG_X86_KERNEL_IBT 1
> +#endif

this autoconf.h business is something I'd rather avoid, it would be
great to be able to use libbpf's __kconfig support to detect
CONFIG_X86_KERNEL_IBT instead? One way would be to mark test6/test7 as
non-auto-loadable (SEC("?...")). Load only test1-tes5, run tests, in
one of BPF programs propagate __kconfig CONFIG_X86_KERNEL_IBT to
user-space through a global variable. Attach skeleton, trigger
everything, remember whether IBT is enabled or not.

If it is defined, load skeleton again, but now enable test6 and test7
and manually attach them through bpf_program__attach_kprobe()
specifying offset as +5 or +9, depending on IBT. It's certainly a bit
more code, but we'll actually test IBT stuff properly.

WDYT?


> +
> +/* test6 and test7 are x86_64 specific because of the instruction
> + * offset, disabling it for all other archs
> + *
> + * CONFIG_X86_KERNEL_IBT adds endbr instruction at function entry,
> + * so disabling test6 and test7, because the offset is hardcoded
> + * in program section
> + */
> +#if !defined(__x86_64__) || defined(CONFIG_X86_KERNEL_IBT)
> +#define DISABLE_OFFSET_ATTACH 1
> +#endif
> +
>  void test_get_func_ip_test(void)
>  {
>         struct get_func_ip_test *skel = NULL;
> @@ -12,10 +30,7 @@ void test_get_func_ip_test(void)
>         if (!ASSERT_OK_PTR(skel, "get_func_ip_test__open"))
>                 return;
>
> -       /* test6 is x86_64 specifc because of the instruction
> -        * offset, disabling it for all other archs
> -        */
> -#ifndef __x86_64__
> +#if defined(DISABLE_OFFSET_ATTACH)
>         bpf_program__set_autoload(skel->progs.test6, false);
>         bpf_program__set_autoload(skel->progs.test7, false);
>  #endif
> @@ -43,7 +58,7 @@ void test_get_func_ip_test(void)
>         ASSERT_EQ(skel->bss->test3_result, 1, "test3_result");
>         ASSERT_EQ(skel->bss->test4_result, 1, "test4_result");
>         ASSERT_EQ(skel->bss->test5_result, 1, "test5_result");
> -#ifdef __x86_64__
> +#if !defined(DISABLE_OFFSET_ATTACH)
>         ASSERT_EQ(skel->bss->test6_result, 1, "test6_result");
>         ASSERT_EQ(skel->bss->test7_result, 1, "test7_result");
>  #endif
> --
> 2.35.3
>

Re: [PATCH bpf-next 4/5] selftests/bpf: Disable kprobe attach test with offset for CONFIG_X86_KERNEL_IBT

[Jiri Olsa]

On Fri, Jul 29, 2022 at 03:15:55PM -0700, Andrii Nakryiko wrote:
> On Sun, Jul 24, 2022 at 2:22 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Attach like 'kprobe/bpf_fentry_test6+0x5' will fail to attach
> > when CONFIG_X86_KERNEL_IBT option is enabled because of the
> > endbr instruction at the function entry.
> >
> > We would need to do manual attach with offset calculation based
> > on the CONFIG_X86_KERNEL_IBT option, which does not seem worth
> > the effort to me.
> >
> > Disabling these test when CONFIG_X86_KERNEL_IBT is enabled.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../bpf/prog_tests/get_func_ip_test.c         | 25 +++++++++++++++----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c b/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
> > index 938dbd4d7c2f..cb0b78fb29df 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
> > @@ -2,6 +2,24 @@
> >  #include <test_progs.h>
> >  #include "get_func_ip_test.skel.h"
> >
> > +/* assume IBT is enabled when kernel configs are not available */
> > +#ifdef HAVE_GENHDR
> > +# include "autoconf.h"
> > +#else
> > +#  define CONFIG_X86_KERNEL_IBT 1
> > +#endif
> 
> this autoconf.h business is something I'd rather avoid, it would be
> great to be able to use libbpf's __kconfig support to detect
> CONFIG_X86_KERNEL_IBT instead? One way would be to mark test6/test7 as
> non-auto-loadable (SEC("?...")). Load only test1-tes5, run tests, in

aah so that's what the '?' prefix is for :))

> one of BPF programs propagate __kconfig CONFIG_X86_KERNEL_IBT to
> user-space through a global variable. Attach skeleton, trigger
> everything, remember whether IBT is enabled or not.
> 
> If it is defined, load skeleton again, but now enable test6 and test7
> and manually attach them through bpf_program__attach_kprobe()
> specifying offset as +5 or +9, depending on IBT. It's certainly a bit
> more code, but we'll actually test IBT stuff properly.
> 
> WDYT?

right, seems doable.. also I wonder how hard would it be to have some
generic support for that, maybe there are other users..  I'll check

thanks,
jirka

[PATCH bpf-next 5/5] selftests/bpf: Fix kprobe get_func_ip tests for CONFIG_X86_KERNEL_IBT

[Jiri Olsa]

The kprobe can be placed anywhere and user must be aware
of the underlying instructions. Therefore fixing just
the bpf program to 'fix' the address to match the actual
function address when CONFIG_X86_KERNEL_IBT is enabled.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/progs/get_func_ip_test.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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..220d56b7c1dc 100644
--- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c
+++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
@@ -2,6 +2,7 @@
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
+#include <stdbool.h>
 
 char _license[] SEC("license") = "GPL";
 
@@ -13,6 +14,8 @@ extern const void bpf_modify_return_test __ksym;
 extern const void bpf_fentry_test6 __ksym;
 extern const void bpf_fentry_test7 __ksym;
 
+extern bool CONFIG_X86_KERNEL_IBT __kconfig __weak;
+
 __u64 test1_result = 0;
 SEC("fentry/bpf_fentry_test1")
 int BPF_PROG(test1, int a)
@@ -37,7 +40,7 @@ __u64 test3_result = 0;
 SEC("kprobe/bpf_fentry_test3")
 int test3(struct pt_regs *ctx)
 {
-	__u64 addr = bpf_get_func_ip(ctx);
+	__u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
 
 	test3_result = (const void *) addr == &bpf_fentry_test3;
 	return 0;
@@ -47,7 +50,7 @@ __u64 test4_result = 0;
 SEC("kretprobe/bpf_fentry_test4")
 int BPF_KRETPROBE(test4)
 {
-	__u64 addr = bpf_get_func_ip(ctx);
+	__u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
 
 	test4_result = (const void *) addr == &bpf_fentry_test4;
 	return 0;
-- 
2.35.3

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

[Andrii Nakryiko]

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.

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?

> 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(-)

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

[Jiri Olsa]

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

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(-)

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

[Jiri Olsa]

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

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

[Andrii Nakryiko]

On Sun, Jul 31, 2022 at 2:08 PM Jiri Olsa <olsajiri@gmail.com> 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

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(-)