All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC bpf-next 0/4] bpf: Fixes for CONFIG_X86_KERNEL_IBT
@ 2022-07-05 19:03 Jiri Olsa
  2022-07-05 19:03 ` [PATCH RFC bpf-next 1/4] bpf: Adjust kprobe_multi entry_ip " Jiri Olsa
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Jiri Olsa @ 2022-07-05 19:03 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Masami Hiramatsu, Martynas Pumputis,
	Yutaro Hayakawa

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.

thanks,
jirka

---
Jiri Olsa (4):
      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 kprobes

 arch/x86/net/bpf_jit_comp.c                               |  9 ++++-----
 kernel/trace/bpf_trace.c                                  |  3 +++
 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, 34 insertions(+), 15 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH RFC bpf-next 1/4] bpf: Adjust kprobe_multi entry_ip for CONFIG_X86_KERNEL_IBT
  2022-07-05 19:03 [PATCH RFC bpf-next 0/4] bpf: Fixes for CONFIG_X86_KERNEL_IBT Jiri Olsa
@ 2022-07-05 19:03 ` Jiri Olsa
  2022-07-05 19:03 ` [PATCH RFC bpf-next 2/4] bpf: Use given function address for trampoline ip arg Jiri Olsa
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Jiri Olsa @ 2022-07-05 19:03 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: Martynas Pumputis, netdev, bpf, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Masami Hiramatsu,
	Yutaro Hayakawa

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.

Reported-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/bpf_trace.c                         | 3 +++
 kernel/trace/ftrace.c                            | 3 +--
 tools/testing/selftests/bpf/progs/kprobe_multi.c | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 4be976cf7d63..ad1e7616c16d 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2419,6 +2419,9 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
 {
 	struct bpf_kprobe_multi_link *link;
 
+#ifdef CONFIG_X86_KERNEL_IBT
+	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/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index e750fe141a60..c866855e77e6 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -8033,8 +8033,7 @@ static int kallsyms_callback(void *data, const char *name,
 	if (!bsearch(&name, args->syms, args->cnt, sizeof(*args->syms), symbols_cmp))
 		return 0;
 
-	addr = ftrace_location(addr);
-	if (!addr)
+	if (!ftrace_location(addr))
 		return 0;
 
 	args->addrs[args->found++] = addr;
diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi.c b/tools/testing/selftests/bpf/progs/kprobe_multi.c
index 93510f4f0f3a..8c6155e17572 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH RFC bpf-next 2/4] bpf: Use given function address for trampoline ip arg
  2022-07-05 19:03 [PATCH RFC bpf-next 0/4] bpf: Fixes for CONFIG_X86_KERNEL_IBT Jiri Olsa
  2022-07-05 19:03 ` [PATCH RFC bpf-next 1/4] bpf: Adjust kprobe_multi entry_ip " Jiri Olsa
@ 2022-07-05 19:03 ` Jiri Olsa
  2022-07-05 19:03 ` [PATCH RFC bpf-next 3/4] selftests/bpf: Disable kprobe attach test with offset for CONFIG_X86_KERNEL_IBT Jiri Olsa
  2022-07-05 19:03 ` [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip tests " Jiri Olsa
  3 siblings, 0 replies; 11+ messages in thread
From: Jiri Olsa @ 2022-07-05 19:03 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Masami Hiramatsu, Martynas Pumputis,
	Yutaro Hayakawa

Using function address given at the generation time as the trampline
ip argument. This way we don't need to read the ip from the stack and
care about CONFIG_X86_KERNEL_IBT option.

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 2f460c67f9c7..1d23dd51a42f 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -2015,13 +2015,14 @@ static bool is_valid_bpf_tramp_flags(unsigned int flags)
 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;
@@ -2097,12 +2098,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


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH RFC bpf-next 3/4] selftests/bpf: Disable kprobe attach test with offset for CONFIG_X86_KERNEL_IBT
  2022-07-05 19:03 [PATCH RFC bpf-next 0/4] bpf: Fixes for CONFIG_X86_KERNEL_IBT Jiri Olsa
  2022-07-05 19:03 ` [PATCH RFC bpf-next 1/4] bpf: Adjust kprobe_multi entry_ip " Jiri Olsa
  2022-07-05 19:03 ` [PATCH RFC bpf-next 2/4] bpf: Use given function address for trampoline ip arg Jiri Olsa
@ 2022-07-05 19:03 ` Jiri Olsa
  2022-07-05 19:03 ` [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip tests " Jiri Olsa
  3 siblings, 0 replies; 11+ messages in thread
From: Jiri Olsa @ 2022-07-05 19:03 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Masami Hiramatsu, Martynas Pumputis,
	Yutaro Hayakawa

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip tests for CONFIG_X86_KERNEL_IBT
  2022-07-05 19:03 [PATCH RFC bpf-next 0/4] bpf: Fixes for CONFIG_X86_KERNEL_IBT Jiri Olsa
                   ` (2 preceding siblings ...)
  2022-07-05 19:03 ` [PATCH RFC bpf-next 3/4] selftests/bpf: Disable kprobe attach test with offset for CONFIG_X86_KERNEL_IBT Jiri Olsa
@ 2022-07-05 19:03 ` Jiri Olsa
  2022-07-06  5:29   ` Andrii Nakryiko
  3 siblings, 1 reply; 11+ messages in thread
From: Jiri Olsa @ 2022-07-05 19:03 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Masami Hiramatsu, Martynas Pumputis,
	Yutaro Hayakawa

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip tests for CONFIG_X86_KERNEL_IBT
  2022-07-05 19:03 ` [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip tests " Jiri Olsa
@ 2022-07-06  5:29   ` Andrii Nakryiko
  2022-07-07 22:16     ` Jiri Olsa
  0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2022-07-06  5:29 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
	bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Masami Hiramatsu, Martynas Pumputis, Yutaro Hayakawa

On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> 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);

so for kprobe bpf_get_func_ip() gets an address with 5 byte
compensation for `call __fentry__`, but not for endr? Why can't we
compensate for endbr inside the kernel code as well? I'd imagine we
either do no compensation (and thus we get &bpf_fentry_test3+5 or
&bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
compensation (and thus always get &bpf_fentry_test3), but this
in-between solution seems to be the worst of both worlds?...

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

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip tests for CONFIG_X86_KERNEL_IBT
  2022-07-06  5:29   ` Andrii Nakryiko
@ 2022-07-07 22:16     ` Jiri Olsa
  2022-07-17 21:43       ` Jiri Olsa
  0 siblings, 1 reply; 11+ messages in thread
From: Jiri Olsa @ 2022-07-07 22:16 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
	bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Masami Hiramatsu, Martynas Pumputis, Yutaro Hayakawa

On Tue, Jul 05, 2022 at 10:29:17PM -0700, Andrii Nakryiko wrote:
> On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > 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);
> 
> so for kprobe bpf_get_func_ip() gets an address with 5 byte
> compensation for `call __fentry__`, but not for endr? Why can't we
> compensate for endbr inside the kernel code as well? I'd imagine we
> either do no compensation (and thus we get &bpf_fentry_test3+5 or
> &bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
> compensation (and thus always get &bpf_fentry_test3), but this
> in-between solution seems to be the worst of both worlds?...

hm rigth, I guess we should be able to do that in bpf_get_func_ip,
I'll check

thanks,
jirka

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

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip tests for CONFIG_X86_KERNEL_IBT
  2022-07-07 22:16     ` Jiri Olsa
@ 2022-07-17 21:43       ` Jiri Olsa
  2022-07-18 11:09         ` Martynas Pumputis
  0 siblings, 1 reply; 11+ messages in thread
From: Jiri Olsa @ 2022-07-17 21:43 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Networking, bpf, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Masami Hiramatsu,
	Martynas Pumputis, Yutaro Hayakawa

On Fri, Jul 08, 2022 at 12:16:35AM +0200, Jiri Olsa wrote:
> On Tue, Jul 05, 2022 at 10:29:17PM -0700, Andrii Nakryiko wrote:
> > On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > 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);
> > 
> > so for kprobe bpf_get_func_ip() gets an address with 5 byte
> > compensation for `call __fentry__`, but not for endr? Why can't we
> > compensate for endbr inside the kernel code as well? I'd imagine we
> > either do no compensation (and thus we get &bpf_fentry_test3+5 or
> > &bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
> > compensation (and thus always get &bpf_fentry_test3), but this
> > in-between solution seems to be the worst of both worlds?...
> 
> hm rigth, I guess we should be able to do that in bpf_get_func_ip,
> I'll check

sorry for late follow up..

so the problem is that you can place kprobe anywhere in the function
(on instruction boundary) but the IBT adjustment of kprobe address is
made only if it's at the function entry and there's endbr instruction

and that kprobe address is what we return in helper:

  BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
  {
        struct kprobe *kp = kprobe_running();

        return kp ? (uintptr_t)kp->addr : 0;
  }

so the adjustment would work only for address at function entry, but
would be wrong for address within the function

perhaps we could add flag to kprobe to indicate the addr adjustment
was done and use it in helper

but that's why I thought I'd keep bpf_get_func_ip_kprobe as it and
leave it up to user

kprobe_multi and trampolines are different, because they can be
only at the function entry, so we can adjust the ip properly

jirka

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip tests for CONFIG_X86_KERNEL_IBT
  2022-07-17 21:43       ` Jiri Olsa
@ 2022-07-18 11:09         ` Martynas Pumputis
  2022-07-18 12:48           ` Jiri Olsa
  0 siblings, 1 reply; 11+ messages in thread
From: Martynas Pumputis @ 2022-07-18 11:09 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Networking, bpf, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Masami Hiramatsu,
	Yutaro Hayakawa



On 7/18/22 00:43, Jiri Olsa wrote:
> On Fri, Jul 08, 2022 at 12:16:35AM +0200, Jiri Olsa wrote:
>> On Tue, Jul 05, 2022 at 10:29:17PM -0700, Andrii Nakryiko wrote:
>>> On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
>>>>
>>>> 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);
>>>
>>> so for kprobe bpf_get_func_ip() gets an address with 5 byte
>>> compensation for `call __fentry__`, but not for endr? Why can't we
>>> compensate for endbr inside the kernel code as well? I'd imagine we
>>> either do no compensation (and thus we get &bpf_fentry_test3+5 or
>>> &bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
>>> compensation (and thus always get &bpf_fentry_test3), but this
>>> in-between solution seems to be the worst of both worlds?...
>>
>> hm rigth, I guess we should be able to do that in bpf_get_func_ip,
>> I'll check
> 
> sorry for late follow up..
> 
> so the problem is that you can place kprobe anywhere in the function
> (on instruction boundary) but the IBT adjustment of kprobe address is
> made only if it's at the function entry and there's endbr instruction

To add more fun to the issue, not all non-inlined functions get endbr64. 
For example "skb_release_head_state()" does, while "skb_free_head()" 
doesn't.

> 
> and that kprobe address is what we return in helper:
> 
>    BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
>    {
>          struct kprobe *kp = kprobe_running();
> 
>          return kp ? (uintptr_t)kp->addr : 0;
>    }
> 
> so the adjustment would work only for address at function entry, but
> would be wrong for address within the function
> 
> perhaps we could add flag to kprobe to indicate the addr adjustment
> was done and use it in helper
> 
> but that's why I thought I'd keep bpf_get_func_ip_kprobe as it and
> leave it up to user
> 
> kprobe_multi and trampolines are different, because they can be
> only at the function entry, so we can adjust the ip properly
> 
> jirka

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip tests for CONFIG_X86_KERNEL_IBT
  2022-07-18 11:09         ` Martynas Pumputis
@ 2022-07-18 12:48           ` Jiri Olsa
  2022-07-19  8:24             ` Jiri Olsa
  0 siblings, 1 reply; 11+ messages in thread
From: Jiri Olsa @ 2022-07-18 12:48 UTC (permalink / raw)
  To: Martynas Pumputis
  Cc: Jiri Olsa, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Networking, bpf, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Masami Hiramatsu,
	Yutaro Hayakawa

On Mon, Jul 18, 2022 at 02:09:54PM +0300, Martynas Pumputis wrote:
> 
> 
> On 7/18/22 00:43, Jiri Olsa wrote:
> > On Fri, Jul 08, 2022 at 12:16:35AM +0200, Jiri Olsa wrote:
> > > On Tue, Jul 05, 2022 at 10:29:17PM -0700, Andrii Nakryiko wrote:
> > > > On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > > 
> > > > > 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);
> > > > 
> > > > so for kprobe bpf_get_func_ip() gets an address with 5 byte
> > > > compensation for `call __fentry__`, but not for endr? Why can't we
> > > > compensate for endbr inside the kernel code as well? I'd imagine we
> > > > either do no compensation (and thus we get &bpf_fentry_test3+5 or
> > > > &bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
> > > > compensation (and thus always get &bpf_fentry_test3), but this
> > > > in-between solution seems to be the worst of both worlds?...
> > > 
> > > hm rigth, I guess we should be able to do that in bpf_get_func_ip,
> > > I'll check
> > 
> > sorry for late follow up..
> > 
> > so the problem is that you can place kprobe anywhere in the function
> > (on instruction boundary) but the IBT adjustment of kprobe address is
> > made only if it's at the function entry and there's endbr instruction
> 
> To add more fun to the issue, not all non-inlined functions get endbr64. For
> example "skb_release_head_state()" does, while "skb_free_head()" doesn't.

ah great.. thanks for info, will check

jirka

> 
> > 
> > and that kprobe address is what we return in helper:
> > 
> >    BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
> >    {
> >          struct kprobe *kp = kprobe_running();
> > 
> >          return kp ? (uintptr_t)kp->addr : 0;
> >    }
> > 
> > so the adjustment would work only for address at function entry, but
> > would be wrong for address within the function
> > 
> > perhaps we could add flag to kprobe to indicate the addr adjustment
> > was done and use it in helper
> > 
> > but that's why I thought I'd keep bpf_get_func_ip_kprobe as it and
> > leave it up to user
> > 
> > kprobe_multi and trampolines are different, because they can be
> > only at the function entry, so we can adjust the ip properly
> > 
> > jirka

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip tests for CONFIG_X86_KERNEL_IBT
  2022-07-18 12:48           ` Jiri Olsa
@ 2022-07-19  8:24             ` Jiri Olsa
  0 siblings, 0 replies; 11+ messages in thread
From: Jiri Olsa @ 2022-07-19  8:24 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Martynas Pumputis, Andrii Nakryiko, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Networking, bpf,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Masami Hiramatsu, Yutaro Hayakawa, Peter Zijlstra

On Mon, Jul 18, 2022 at 02:48:46PM +0200, Jiri Olsa wrote:
> On Mon, Jul 18, 2022 at 02:09:54PM +0300, Martynas Pumputis wrote:
> > 
> > 
> > On 7/18/22 00:43, Jiri Olsa wrote:
> > > On Fri, Jul 08, 2022 at 12:16:35AM +0200, Jiri Olsa wrote:
> > > > On Tue, Jul 05, 2022 at 10:29:17PM -0700, Andrii Nakryiko wrote:
> > > > > On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > > > 
> > > > > > 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);
> > > > > 
> > > > > so for kprobe bpf_get_func_ip() gets an address with 5 byte
> > > > > compensation for `call __fentry__`, but not for endr? Why can't we
> > > > > compensate for endbr inside the kernel code as well? I'd imagine we
> > > > > either do no compensation (and thus we get &bpf_fentry_test3+5 or
> > > > > &bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
> > > > > compensation (and thus always get &bpf_fentry_test3), but this
> > > > > in-between solution seems to be the worst of both worlds?...
> > > > 
> > > > hm rigth, I guess we should be able to do that in bpf_get_func_ip,
> > > > I'll check
> > > 
> > > sorry for late follow up..
> > > 
> > > so the problem is that you can place kprobe anywhere in the function
> > > (on instruction boundary) but the IBT adjustment of kprobe address is
> > > made only if it's at the function entry and there's endbr instruction
> > 
> > To add more fun to the issue, not all non-inlined functions get endbr64. For
> > example "skb_release_head_state()" does, while "skb_free_head()" doesn't.
> 
> ah great.. thanks for info, will check

I checked with Peter and yes the endbr does not need to be there

<peterz> IBT is 'Indirect Branch Tracking' ENDBR needs to be at the target for "JMP *%reg" and "CALL *%reg"
<peterz> direct call/jmp don't need anything specal

so we will need to hold the +4 info somewhere for each address
and use that in get_func_ip helper or perhaps we could read
previous instruction and check if the previous instruction is
endbr with check like:

	if (is_endbr(*(u32 *)(addr - 4)))
		addr -= 4

jirka

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-07-19  8:24 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-05 19:03 [PATCH RFC bpf-next 0/4] bpf: Fixes for CONFIG_X86_KERNEL_IBT Jiri Olsa
2022-07-05 19:03 ` [PATCH RFC bpf-next 1/4] bpf: Adjust kprobe_multi entry_ip " Jiri Olsa
2022-07-05 19:03 ` [PATCH RFC bpf-next 2/4] bpf: Use given function address for trampoline ip arg Jiri Olsa
2022-07-05 19:03 ` [PATCH RFC bpf-next 3/4] selftests/bpf: Disable kprobe attach test with offset for CONFIG_X86_KERNEL_IBT Jiri Olsa
2022-07-05 19:03 ` [PATCH RFC bpf-next 4/4] selftests/bpf: Fix kprobe get_func_ip tests " Jiri Olsa
2022-07-06  5:29   ` Andrii Nakryiko
2022-07-07 22:16     ` Jiri Olsa
2022-07-17 21:43       ` Jiri Olsa
2022-07-18 11:09         ` Martynas Pumputis
2022-07-18 12:48           ` Jiri Olsa
2022-07-19  8:24             ` Jiri Olsa

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.