[PATCHv4 bpf-next 0/6] bpf: Fixes for CONFIG_X86_KERNEL_IBT

[PATCHv4 bpf-next 0/6] 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.

v4 changes:
  - used get_kernel_nofault to read previous instruction [Peter]
  - used movabs instruction in trampoline comment [Peter]
  - renamed fentry_ip argument in kprobe_multi_link_handler [Peter]

v3 changes:
  - using 'unused' bpf function to get IBT config option
    into selftest skeleton
  - rebased to current bpf-next/master
  - added ack/review from Masami

v2 changes:
  - change kprobes get_func_ip to return zero for kprobes
    attached within the function body [Andrii]
  - detect IBT config and properly test kprobe with offset 
    [Andrii]

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 (6):
      kprobes: Add new KPROBE_FLAG_ON_FUNC_ENTRY kprobe flag
      ftrace: Keep the resolved addr in kallsyms_callback
      bpf: Use given function address for trampoline ip arg
      bpf: Adjust kprobe_multi entry_ip for CONFIG_X86_KERNEL_IBT
      bpf: Return value in kprobe get_func_ip only for entry address
      selftests/bpf: Fix get_func_ip offset test for CONFIG_X86_KERNEL_IBT

 arch/x86/net/bpf_jit_comp.c                               | 11 +++++------
 include/linux/kprobes.h                                   |  1 +
 kernel/kprobes.c                                          |  6 +++++-
 kernel/trace/bpf_trace.c                                  | 25 ++++++++++++++++++++++---
 kernel/trace/ftrace.c                                     |  3 +--
 tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
 tools/testing/selftests/bpf/progs/get_func_ip_test.c      | 25 +++++++++++++------------
 tools/testing/selftests/bpf/progs/kprobe_multi.c          |  4 +---
 8 files changed, 96 insertions(+), 38 deletions(-)

[PATCHv4 bpf-next 1/6] kprobes: Add new KPROBE_FLAG_ON_FUNC_ENTRY kprobe flag

[Jiri Olsa]

Adding KPROBE_FLAG_ON_FUNC_ENTRY kprobe flag to indicate that
attach address is on function entry. This is used in following
changes in get_func_ip helper to return correct function address.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/kprobes.h | 1 +
 kernel/kprobes.c        | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

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 08350e35aba2..51adc3c94503 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1606,9 +1606,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;
@@ -1628,6 +1629,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'. */
-- 
2.37.3

[PATCHv4 bpf-next 2/6] 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);
               ...

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
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 439e2ab6905e..447d2e2a8549 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -8265,8 +8265,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.37.3

[PATCHv4 bpf-next 3/6] 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 | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index ae89f4143eb4..d4a6183197e9 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -662,7 +662,7 @@ static void emit_mov_imm64(u8 **pprog, u32 dst_reg,
 		 */
 		emit_mov_imm32(&prog, false, dst_reg, imm32_lo);
 	} else {
-		/* movabsq %rax, imm64 */
+		/* movabsq rax, imm64 */
 		EMIT2(add_1mod(0x48, dst_reg), add_1reg(0xB8, dst_reg));
 		EMIT(imm32_lo, 4);
 		EMIT(imm32_hi, 4);
@@ -2039,13 +2039,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, extra_nregs = 0;
 	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;
@@ -2126,12 +2127,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
+		 * movabsq 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.37.3

[PATCHv4 bpf-next 4/6] 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 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.

Also renaming kprobe_multi_link_handler entry_ip argument to fentry_ip
so it's clearer this is an ftrace __fentry__ ip.

[1] commit 7f0059b58f02 ("selftests/bpf: Fix kprobe_multi test.")

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                      | 20 +++++++++++++++++--
 .../selftests/bpf/progs/kprobe_multi.c        |  4 +---
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index b05f0310dbd3..ebd1b348beb3 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1028,6 +1028,22 @@ static const struct bpf_func_proto bpf_get_func_ip_proto_tracing = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+#ifdef CONFIG_X86_KERNEL_IBT
+static unsigned long get_entry_ip(unsigned long fentry_ip)
+{
+	u32 instr;
+
+	/* Being extra safe in here in case entry ip is on the page-edge. */
+	if (get_kernel_nofault(instr, (u32 *) fentry_ip - 1))
+		return fentry_ip;
+	if (is_endbr(instr))
+		fentry_ip -= ENDBR_INSN_SIZE;
+	return fentry_ip;
+}
+#else
+#define get_entry_ip(fentry_ip) fentry_ip
+#endif
+
 BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
 {
 	struct kprobe *kp = kprobe_running();
@@ -2600,13 +2616,13 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
 }
 
 static void
-kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
+kprobe_multi_link_handler(struct fprobe *fp, unsigned long fentry_ip,
 			  struct pt_regs *regs)
 {
 	struct bpf_kprobe_multi_link *link;
 
 	link = container_of(fp, struct bpf_kprobe_multi_link, fp);
-	kprobe_multi_link_prog_run(link, entry_ip, regs);
+	kprobe_multi_link_prog_run(link, get_entry_ip(fentry_ip), regs);
 }
 
 static int symbols_cmp_r(const void *a, const void *b, const void *priv)
diff --git a/tools/testing/selftests/bpf/progs/kprobe_multi.c b/tools/testing/selftests/bpf/progs/kprobe_multi.c
index 08f95a8155d1..98c3399e15c0 100644
--- a/tools/testing/selftests/bpf/progs/kprobe_multi.c
+++ b/tools/testing/selftests/bpf/progs/kprobe_multi.c
@@ -36,15 +36,13 @@ __u64 kretprobe_test6_result = 0;
 __u64 kretprobe_test7_result = 0;
 __u64 kretprobe_test8_result = 0;
 
-extern bool CONFIG_X86_KERNEL_IBT __kconfig __weak;
-
 static void kprobe_multi_check(void *ctx, bool is_return)
 {
 	if (bpf_get_current_pid_tgid() >> 32 != pid)
 		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.37.3

[PATCHv4 bpf-next 5/6] bpf: Return value in kprobe get_func_ip only for entry address

[Jiri Olsa]

Changing return value of kprobe's version of bpf_get_func_ip
to return zero if the attach address is not on the function's
entry point.

For kprobes attached in the middle of the function we can't easily
get to the function address especially now with the CONFIG_X86_KERNEL_IBT
support.

If user cares about current IP for kprobes attached within the
function body, they can get it with PT_REGS_IP(ctx).

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/bpf_trace.c                             | 5 ++++-
 tools/testing/selftests/bpf/progs/get_func_ip_test.c | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index ebd1b348beb3..688552df95ca 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1048,7 +1048,10 @@ BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
 {
 	struct kprobe *kp = kprobe_running();
 
-	return kp ? (uintptr_t)kp->addr : 0;
+	if (!kp || !(kp->flags & KPROBE_FLAG_ON_FUNC_ENTRY))
+		return 0;
+
+	return get_entry_ip((uintptr_t)kp->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;
 }
-- 
2.37.3

[PATCHv4 bpf-next 6/6] selftests/bpf: Fix get_func_ip offset test for CONFIG_X86_KERNEL_IBT

[Jiri Olsa]

With CONFIG_X86_KERNEL_IBT enabled the test for kprobe with offset
won't work because of the extra endbr instruction.

As suggested by Andrii adding CONFIG_X86_KERNEL_IBT detection
and using appropriate offset value based on that.

Also removing test7 program, because it does the same as test6.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/prog_tests/get_func_ip_test.c         | 59 +++++++++++++++----
 .../selftests/bpf/progs/get_func_ip_test.c    | 23 ++++----
 2 files changed, 60 insertions(+), 22 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..fede8ef58b5b 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,7 +2,7 @@
 #include <test_progs.h>
 #include "get_func_ip_test.skel.h"
 
-void test_get_func_ip_test(void)
+static void test_function_entry(void)
 {
 	struct get_func_ip_test *skel = NULL;
 	int err, prog_fd;
@@ -12,14 +12,6 @@ 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__
-	bpf_program__set_autoload(skel->progs.test6, false);
-	bpf_program__set_autoload(skel->progs.test7, false);
-#endif
-
 	err = get_func_ip_test__load(skel);
 	if (!ASSERT_OK(err, "get_func_ip_test__load"))
 		goto cleanup;
@@ -43,11 +35,56 @@ 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");
+
+cleanup:
+	get_func_ip_test__destroy(skel);
+}
+
+/* test6 is x86_64 specific because of the instruction
+ * offset, disabling it for all other archs
+ */
 #ifdef __x86_64__
+static void test_function_body(void)
+{
+	struct get_func_ip_test *skel = NULL;
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	LIBBPF_OPTS(bpf_kprobe_opts, kopts);
+	struct bpf_link *link6 = NULL;
+	int err, prog_fd;
+
+	skel = get_func_ip_test__open();
+	if (!ASSERT_OK_PTR(skel, "get_func_ip_test__open"))
+		return;
+
+	bpf_program__set_autoload(skel->progs.test6, true);
+
+	err = get_func_ip_test__load(skel);
+	if (!ASSERT_OK(err, "get_func_ip_test__load"))
+		goto cleanup;
+
+	kopts.offset = skel->kconfig->CONFIG_X86_KERNEL_IBT ? 9 : 5;
+
+	link6 = bpf_program__attach_kprobe_opts(skel->progs.test6, "bpf_fentry_test6", &kopts);
+	if (!ASSERT_OK_PTR(link6, "link6"))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(skel->progs.test1);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	ASSERT_OK(err, "test_run");
+	ASSERT_EQ(topts.retval, 0, "test_run");
+
 	ASSERT_EQ(skel->bss->test6_result, 1, "test6_result");
-	ASSERT_EQ(skel->bss->test7_result, 1, "test7_result");
-#endif
 
 cleanup:
+	bpf_link__destroy(link6);
 	get_func_ip_test__destroy(skel);
 }
+#else
+#define test_function_body()
+#endif
+
+void test_get_func_ip_test(void)
+{
+	test_function_entry();
+	test_function_body();
+}
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 6db70757bc8b..8559e698b40d 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,16 @@ 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;
+
+/* This function is here to have CONFIG_X86_KERNEL_IBT
+ * used and added to object BTF.
+ */
+int unused(void)
+{
+	return CONFIG_X86_KERNEL_IBT ? 0 : 1;
+}
+
 __u64 test1_result = 0;
 SEC("fentry/bpf_fentry_test1")
 int BPF_PROG(test1, int a)
@@ -64,7 +75,7 @@ int BPF_PROG(test5, int a, int *b, int ret)
 }
 
 __u64 test6_result = 0;
-SEC("kprobe/bpf_fentry_test6+0x5")
+SEC("?kprobe")
 int test6(struct pt_regs *ctx)
 {
 	__u64 addr = bpf_get_func_ip(ctx);
@@ -72,13 +83,3 @@ int test6(struct pt_regs *ctx)
 	test6_result = (const void *) addr == 0;
 	return 0;
 }
-
-__u64 test7_result = 0;
-SEC("kprobe/bpf_fentry_test7+5")
-int test7(struct pt_regs *ctx)
-{
-	__u64 addr = bpf_get_func_ip(ctx);
-
-	test7_result = (const void *) addr == 0;
-	return 0;
-}
-- 
2.37.3

Re: [PATCHv4 bpf-next 4/6] bpf: Adjust kprobe_multi entry_ip for CONFIG_X86_KERNEL_IBT

[Andrii Nakryiko]

On Thu, Sep 22, 2022 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> 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 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.
>
> Also renaming kprobe_multi_link_handler entry_ip argument to fentry_ip
> so it's clearer this is an ftrace __fentry__ ip.
>
> [1] commit 7f0059b58f02 ("selftests/bpf: Fix kprobe_multi test.")
>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Reported-by: Martynas Pumputis <m@lambda.lt>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

lgtm

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  kernel/trace/bpf_trace.c                      | 20 +++++++++++++++++--
>  .../selftests/bpf/progs/kprobe_multi.c        |  4 +---
>  2 files changed, 19 insertions(+), 5 deletions(-)
>

[...]

Re: [PATCHv4 bpf-next 5/6] bpf: Return value in kprobe get_func_ip only for entry address

[Andrii Nakryiko]

On Thu, Sep 22, 2022 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Changing return value of kprobe's version of bpf_get_func_ip
> to return zero if the attach address is not on the function's
> entry point.
>
> For kprobes attached in the middle of the function we can't easily
> get to the function address especially now with the CONFIG_X86_KERNEL_IBT
> support.
>
> If user cares about current IP for kprobes attached within the
> function body, they can get it with PT_REGS_IP(ctx).
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/trace/bpf_trace.c                             | 5 ++++-
>  tools/testing/selftests/bpf/progs/get_func_ip_test.c | 4 ++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
>

Can you please add a note in bpf_get_func_ip() description in
uapi/linux/bpf.h that this function returns zero for kprobes in the
middle of the function?

With that:

Acked-by: Andrii Nakryiko <andrii@kernel.org>


> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index ebd1b348beb3..688552df95ca 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1048,7 +1048,10 @@ BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
>  {
>         struct kprobe *kp = kprobe_running();
>
> -       return kp ? (uintptr_t)kp->addr : 0;
> +       if (!kp || !(kp->flags & KPROBE_FLAG_ON_FUNC_ENTRY))
> +               return 0;
> +
> +       return get_entry_ip((uintptr_t)kp->addr);
>  }

[...]

Re: [PATCHv4 bpf-next 6/6] selftests/bpf: Fix get_func_ip offset test for CONFIG_X86_KERNEL_IBT

[Andrii Nakryiko]

On Thu, Sep 22, 2022 at 2:05 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> With CONFIG_X86_KERNEL_IBT enabled the test for kprobe with offset
> won't work because of the extra endbr instruction.
>
> As suggested by Andrii adding CONFIG_X86_KERNEL_IBT detection
> and using appropriate offset value based on that.
>
> Also removing test7 program, because it does the same as test6.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../bpf/prog_tests/get_func_ip_test.c         | 59 +++++++++++++++----
>  .../selftests/bpf/progs/get_func_ip_test.c    | 23 ++++----
>  2 files changed, 60 insertions(+), 22 deletions(-)
>

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> 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..fede8ef58b5b 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,7 +2,7 @@
>  #include <test_progs.h>
>  #include "get_func_ip_test.skel.h"
>

[...]

Re: [PATCHv4 bpf-next 5/6] bpf: Return value in kprobe get_func_ip only for entry address

[Martynas Pumputis]

On 9/22/22 23:03, Jiri Olsa wrote:
> Changing return value of kprobe's version of bpf_get_func_ip
> to return zero if the attach address is not on the function's
> entry point.
> 
> For kprobes attached in the middle of the function we can't easily
> get to the function address especially now with the CONFIG_X86_KERNEL_IBT
> support.
> 
> If user cares about current IP for kprobes attached within the
> function body, they can get it with PT_REGS_IP(ctx).
> 
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Tested the patches with "pwru --filter-func='.*skb.*' 
--filter-dst-ip=1.1.1.1" from [1] - the symbol name resolution works, 
thanks!

Without your patches:

                SKB    CPU          PROCESS                     FUNC
0xffff8989c159b4e8      0           [curl]       0xffffffffbbb06164
0xffff8989c223f000      0           [curl]       0xffffffffbbb07534
0xffff8989c223f000      0           [curl]       0xffffffffbbb04934
0xffff8989c223f000      0           [curl]         skb_release_data
0xffff8989c223f000      0           [curl]             kfree_skbmem
0xffff8989c159b4e8      0           [curl]       0xffffffffbbb00db4
[..]

With patches:

                SKB    CPU          PROCESS                     FUNC
0xffffa4564159b4e8      0           [curl]   validate_xmit_skb_list
0xffffa4564159b4e8      0           [curl]       netif_skb_features
0xffffa4564159b4e8      0           [curl]     skb_network_protocol
0xffffa4564159b4e8      0           [curl]  skb_csum_hwoffload_help
0xffffa4564159b4e8      0           [curl]        skb_checksum_help
0xffffa4564159b4e8      0           [curl]      skb_ensure_writable
0xffffa4564159b4e8      0           [curl]             skb_to_sgvec
[..]

[1]: https://github.com/cilium/pwru/tree/test-ibt-kernel-fix

Acked-by: Martynas Pumputis <m@lambda.lt>

> ---
>   kernel/trace/bpf_trace.c                             | 5 ++++-
>   tools/testing/selftests/bpf/progs/get_func_ip_test.c | 4 ++--
>   2 files changed, 6 insertions(+), 3 deletions(-)

[..]

Re: [PATCHv4 bpf-next 5/6] bpf: Return value in kprobe get_func_ip only for entry address

[Jiri Olsa]

On Fri, Sep 23, 2022 at 02:42:07PM -0700, Andrii Nakryiko wrote:
> On Thu, Sep 22, 2022 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Changing return value of kprobe's version of bpf_get_func_ip
> > to return zero if the attach address is not on the function's
> > entry point.
> >
> > For kprobes attached in the middle of the function we can't easily
> > get to the function address especially now with the CONFIG_X86_KERNEL_IBT
> > support.
> >
> > If user cares about current IP for kprobes attached within the
> > function body, they can get it with PT_REGS_IP(ctx).
> >
> > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/trace/bpf_trace.c                             | 5 ++++-
> >  tools/testing/selftests/bpf/progs/get_func_ip_test.c | 4 ++--
> >  2 files changed, 6 insertions(+), 3 deletions(-)
> >
> 
> Can you please add a note in bpf_get_func_ip() description in
> uapi/linux/bpf.h that this function returns zero for kprobes in the
> middle of the function?

yep, will do that

thanks,
jirka

> 
> With that:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> 
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index ebd1b348beb3..688552df95ca 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -1048,7 +1048,10 @@ BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
> >  {
> >         struct kprobe *kp = kprobe_running();
> >
> > -       return kp ? (uintptr_t)kp->addr : 0;
> > +       if (!kp || !(kp->flags & KPROBE_FLAG_ON_FUNC_ENTRY))
> > +               return 0;
> > +
> > +       return get_entry_ip((uintptr_t)kp->addr);
> >  }
> 
> [...]

Re: [PATCHv4 bpf-next 5/6] bpf: Return value in kprobe get_func_ip only for entry address

[Jiri Olsa]

On Sun, Sep 25, 2022 at 07:42:48AM +0200, Martynas Pumputis wrote:
> On 9/22/22 23:03, Jiri Olsa wrote:
> > Changing return value of kprobe's version of bpf_get_func_ip
> > to return zero if the attach address is not on the function's
> > entry point.
> > 
> > For kprobes attached in the middle of the function we can't easily
> > get to the function address especially now with the CONFIG_X86_KERNEL_IBT
> > support.
> > 
> > If user cares about current IP for kprobes attached within the
> > function body, they can get it with PT_REGS_IP(ctx).
> > 
> > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Tested the patches with "pwru --filter-func='.*skb.*'
> --filter-dst-ip=1.1.1.1" from [1] - the symbol name resolution works,
> thanks!
> 
> Without your patches:
> 
>                SKB    CPU          PROCESS                     FUNC
> 0xffff8989c159b4e8      0           [curl]       0xffffffffbbb06164
> 0xffff8989c223f000      0           [curl]       0xffffffffbbb07534
> 0xffff8989c223f000      0           [curl]       0xffffffffbbb04934
> 0xffff8989c223f000      0           [curl]         skb_release_data
> 0xffff8989c223f000      0           [curl]             kfree_skbmem
> 0xffff8989c159b4e8      0           [curl]       0xffffffffbbb00db4
> [..]
> 
> With patches:
> 
>                SKB    CPU          PROCESS                     FUNC
> 0xffffa4564159b4e8      0           [curl]   validate_xmit_skb_list
> 0xffffa4564159b4e8      0           [curl]       netif_skb_features
> 0xffffa4564159b4e8      0           [curl]     skb_network_protocol
> 0xffffa4564159b4e8      0           [curl]  skb_csum_hwoffload_help
> 0xffffa4564159b4e8      0           [curl]        skb_checksum_help
> 0xffffa4564159b4e8      0           [curl]      skb_ensure_writable
> 0xffffa4564159b4e8      0           [curl]             skb_to_sgvec
> [..]
> 
> [1]: https://github.com/cilium/pwru/tree/test-ibt-kernel-fix
> 
> Acked-by: Martynas Pumputis <m@lambda.lt>

awesome, thanks

jirka

> 
> > ---
> >   kernel/trace/bpf_trace.c                             | 5 ++++-
> >   tools/testing/selftests/bpf/progs/get_func_ip_test.c | 4 ++--
> >   2 files changed, 6 insertions(+), 3 deletions(-)
> 
> [..]