[PATCH bpf-next 0/3] uprobe: uretprobe speed up

[PATCH bpf-next 0/3] uprobe: uretprobe speed up

[Jiri Olsa]

hi,
as part of the effort on speeding up the uprobes [0] coming with
return uprobe optimization by using syscall instead of the trap
on the uretprobe trampoline.

The speed up depends on instruction type that uprobe is installed
and depends on specific HW type, please check patch 1 for details.

I added extra bpf selftest to check on registers values before and
after uretprobe to make sure the syscall saves all the needed regs.

changes from RFC:
  - using __NR_uretprobe instead hardcoded syscall value [Andrii]
  - using __builtin_memcpy to copy registers [Andrii]
  - adding more comments
  - Oleg provided the code that enabled the syscall to take the
    sysret path, which made it lot faster [Oleg]
  - nocf_check attribute change was sent separatelly
  - added test for checking up changed registers in uprobe consumer

thanks,
jirka


[0] https://lore.kernel.org/bpf/ZeCXHKJ--iYYbmLj@krava/
---
Jiri Olsa (3):
      uprobe: Add uretprobe syscall to speed up return probe
      selftests/bpf: Add uretprobe syscall test for regs integrity
      selftests/bpf: Add uretprobe syscall test for regs changes

 arch/x86/entry/syscalls/syscall_64.tbl                  |   1 +
 arch/x86/kernel/uprobes.c                               |  83 ++++++++++++++++++++++++++++++++++
 include/linux/syscalls.h                                |   2 +
 include/linux/uprobes.h                                 |   2 +
 include/uapi/asm-generic/unistd.h                       |   5 ++-
 kernel/events/uprobes.c                                 |  18 ++++++--
 kernel/sys_ni.c                                         |   2 +
 tools/include/linux/compiler.h                          |   4 ++
 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c   | 119 ++++++++++++++++++++++++++++++++++++++++++++++++-
 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c | 230 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/uprobe_syscall.c      |  15 +++++++
 11 files changed, 475 insertions(+), 6 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
 create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c

[PATCH bpf-next 1/3] uprobe: Add uretprobe syscall to speed up return probe

[Jiri Olsa]

Adding uretprobe syscall instead of trap to speed up return probe.

At the moment the uretprobe setup/path is:

  - install entry uprobe

  - when the uprobe is hit, it overwrites probed function's return address
    on stack with address of the trampoline that contains breakpoint
    instruction

  - the breakpoint trap code handles the uretprobe consumers execution and
    jumps back to original return address

This patch replaces the above trampoline's breakpoint instruction with new
ureprobe syscall call. This syscall does exactly the same job as the trap
with some more extra work:

  - syscall trampoline must save original value for rax/r11/rcx registers
    on stack - rax is set to syscall number and r11/rcx are changed and
    used by syscall instruction

  - the syscall code reads the original values of those registers and
    restore those values in task's pt_regs area

Even with the extra registers handling code the having uretprobes handled
by syscalls shows speed improvement.

  On Intel (11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz)

  current:

    base           :   15.888 ± 0.033M/s
    uprobe-nop     :    3.016 ± 0.000M/s
    uprobe-push    :    2.832 ± 0.005M/s
    uprobe-ret     :    1.104 ± 0.000M/s
    uretprobe-nop  :    1.487 ± 0.000M/s
    uretprobe-push :    1.456 ± 0.000M/s
    uretprobe-ret  :    0.816 ± 0.001M/s

  with the fix:

    base           :   15.116 ± 0.045M/s
    uprobe-nop     :    3.001 ± 0.045M/s
    uprobe-push    :    2.831 ± 0.004M/s
    uprobe-ret     :    1.102 ± 0.001M/s
    uretprobe-nop  :    1.969 ± 0.001M/s  < 32% speedup
    uretprobe-push :    1.905 ± 0.004M/s  < 30% speedup
    uretprobe-ret  :    0.933 ± 0.002M/s  < 14% speedup

  On Amd (AMD Ryzen 7 5700U)

  current:

    base           :    5.105 ± 0.003M/s
    uprobe-nop     :    1.552 ± 0.002M/s
    uprobe-push    :    1.408 ± 0.003M/s
    uprobe-ret     :    0.827 ± 0.001M/s
    uretprobe-nop  :    0.779 ± 0.001M/s
    uretprobe-push :    0.750 ± 0.001M/s
    uretprobe-ret  :    0.539 ± 0.001M/s

  with the fix:

    base           :    5.119 ± 0.002M/s
    uprobe-nop     :    1.523 ± 0.003M/s
    uprobe-push    :    1.384 ± 0.003M/s
    uprobe-ret     :    0.826 ± 0.002M/s
    uretprobe-nop  :    0.866 ± 0.002M/s  < 11% speedup
    uretprobe-push :    0.826 ± 0.002M/s  < 10% speedup
    uretprobe-ret  :    0.581 ± 0.001M/s  <  7% speedup

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/entry/syscalls/syscall_64.tbl |  1 +
 arch/x86/kernel/uprobes.c              | 83 ++++++++++++++++++++++++++
 include/linux/syscalls.h               |  2 +
 include/linux/uprobes.h                |  2 +
 include/uapi/asm-generic/unistd.h      |  5 +-
 kernel/events/uprobes.c                | 18 ++++--
 kernel/sys_ni.c                        |  2 +
 7 files changed, 108 insertions(+), 5 deletions(-)

diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 7e8d46f4147f..af0a33ab06ee 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -383,6 +383,7 @@
 459	common	lsm_get_self_attr	sys_lsm_get_self_attr
 460	common	lsm_set_self_attr	sys_lsm_set_self_attr
 461	common	lsm_list_modules	sys_lsm_list_modules
+462	64	uretprobe		sys_uretprobe
 
 #
 # Due to a historical design error, certain syscalls are numbered differently
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 6c07f6daaa22..6fc5d16f6c17 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -12,6 +12,7 @@
 #include <linux/ptrace.h>
 #include <linux/uprobes.h>
 #include <linux/uaccess.h>
+#include <linux/syscalls.h>
 
 #include <linux/kdebug.h>
 #include <asm/processor.h>
@@ -308,6 +309,88 @@ static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool
 }
 
 #ifdef CONFIG_X86_64
+
+asm (
+	".pushsection .rodata\n"
+	".global uretprobe_syscall_entry\n"
+	"uretprobe_syscall_entry:\n"
+	"pushq %rax\n"
+	"pushq %rcx\n"
+	"pushq %r11\n"
+	"movq $" __stringify(__NR_uretprobe) ", %rax\n"
+	"syscall\n"
+	"popq %r11\n"
+	"popq %rcx\n"
+
+	/* The uretprobe syscall replaces stored %rax value with final
+	 * return address, so we don't restore %rax in here and just
+	 * call ret.
+	 */
+	"retq\n"
+	".global uretprobe_syscall_end\n"
+	"uretprobe_syscall_end:\n"
+	".popsection\n"
+);
+
+extern u8 uretprobe_syscall_entry[];
+extern u8 uretprobe_syscall_end[];
+
+void *arch_uprobe_trampoline(unsigned long *psize)
+{
+	*psize = uretprobe_syscall_end - uretprobe_syscall_entry;
+	return uretprobe_syscall_entry;
+}
+
+SYSCALL_DEFINE0(uretprobe)
+{
+	struct pt_regs *regs = task_pt_regs(current);
+	unsigned long err, ip, sp, r11_cx_ax[3];
+
+	err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
+	WARN_ON_ONCE(err);
+
+	/* expose the "right" values of r11/cx/ax/sp to uprobe_consumer/s */
+	regs->r11 = r11_cx_ax[0];
+	regs->cx  = r11_cx_ax[1];
+	regs->ax  = r11_cx_ax[2];
+	regs->sp += sizeof(r11_cx_ax);
+	regs->orig_ax = -1;
+
+	ip = regs->ip;
+	sp = regs->sp;
+
+	uprobe_handle_trampoline(regs);
+
+	/*
+	 * uprobe_consumer has changed sp, we can do nothing,
+	 * just return via iret
+	 */
+	if (regs->sp != sp)
+		return regs->ax;
+	regs->sp -= sizeof(r11_cx_ax);
+
+	/* for the case uprobe_consumer has changed r11/cx */
+	r11_cx_ax[0] = regs->r11;
+	r11_cx_ax[1] = regs->cx;
+
+	/*
+	 * ax register is passed through as return value, so we can use
+	 * its space on stack for ip value and jump to it through the
+	 * trampoline's ret instruction
+	 */
+	r11_cx_ax[2] = regs->ip;
+	regs->ip = ip;
+
+	err = copy_to_user((void __user *)regs->sp, r11_cx_ax, sizeof(r11_cx_ax));
+	WARN_ON_ONCE(err);
+
+	/* ensure sysret, see do_syscall_64() */
+	regs->r11 = regs->flags;
+	regs->cx  = regs->ip;
+
+	return regs->ax;
+}
+
 /*
  * If arch_uprobe->insn doesn't use rip-relative addressing, return
  * immediately.  Otherwise, rewrite the instruction so that it accesses
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 77eb9b0e7685..db150794f89d 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -972,6 +972,8 @@ asmlinkage long sys_lsm_list_modules(u64 *ids, size_t *size, u32 flags);
 /* x86 */
 asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int on);
 
+asmlinkage long sys_uretprobe(void);
+
 /* pciconfig: alpha, arm, arm64, ia64, sparc */
 asmlinkage long sys_pciconfig_read(unsigned long bus, unsigned long dfn,
 				unsigned long off, unsigned long len,
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index f46e0ca0169c..a490146ad89d 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -138,6 +138,8 @@ extern bool arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check c
 extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
 extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
 					 void *src, unsigned long len);
+extern void uprobe_handle_trampoline(struct pt_regs *regs);
+extern void *arch_uprobe_trampoline(unsigned long *psize);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 75f00965ab15..8a747cd1d735 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -842,8 +842,11 @@ __SYSCALL(__NR_lsm_set_self_attr, sys_lsm_set_self_attr)
 #define __NR_lsm_list_modules 461
 __SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
 
+#define __NR_uretprobe 462
+__SYSCALL(__NR_uretprobe, sys_uretprobe)
+
 #undef __NR_syscalls
-#define __NR_syscalls 462
+#define __NR_syscalls 463
 
 /*
  * 32 bit systems traditionally used different
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 929e98c62965..90395b16bde0 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1474,11 +1474,20 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
 	return ret;
 }
 
+void * __weak arch_uprobe_trampoline(unsigned long *psize)
+{
+	static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+
+	*psize = UPROBE_SWBP_INSN_SIZE;
+	return &insn;
+}
+
 static struct xol_area *__create_xol_area(unsigned long vaddr)
 {
 	struct mm_struct *mm = current->mm;
-	uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+	unsigned long insns_size;
 	struct xol_area *area;
+	void *insns;
 
 	area = kmalloc(sizeof(*area), GFP_KERNEL);
 	if (unlikely(!area))
@@ -1502,7 +1511,8 @@ static struct xol_area *__create_xol_area(unsigned long vaddr)
 	/* Reserve the 1st slot for get_trampoline_vaddr() */
 	set_bit(0, area->bitmap);
 	atomic_set(&area->slot_count, 1);
-	arch_uprobe_copy_ixol(area->pages[0], 0, &insn, UPROBE_SWBP_INSN_SIZE);
+	insns = arch_uprobe_trampoline(&insns_size);
+	arch_uprobe_copy_ixol(area->pages[0], 0, insns, insns_size);
 
 	if (!xol_add_vma(mm, area))
 		return area;
@@ -2123,7 +2133,7 @@ static struct return_instance *find_next_ret_chain(struct return_instance *ri)
 	return ri;
 }
 
-static void handle_trampoline(struct pt_regs *regs)
+void uprobe_handle_trampoline(struct pt_regs *regs)
 {
 	struct uprobe_task *utask;
 	struct return_instance *ri, *next;
@@ -2188,7 +2198,7 @@ static void handle_swbp(struct pt_regs *regs)
 
 	bp_vaddr = uprobe_get_swbp_addr(regs);
 	if (bp_vaddr == get_trampoline_vaddr())
-		return handle_trampoline(regs);
+		return uprobe_handle_trampoline(regs);
 
 	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
 	if (!uprobe) {
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index faad00cce269..be6195e0d078 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -391,3 +391,5 @@ COND_SYSCALL(setuid16);
 
 /* restartable sequence */
 COND_SYSCALL(rseq);
+
+COND_SYSCALL(uretprobe);
-- 
2.44.0

[PATCH bpf-next 2/3] selftests/bpf: Add uretprobe syscall test for regs integrity

[Jiri Olsa]

Add uretprobe syscall test that compares register values before
and after the uretprobe is hit. It also compares the register
values seen from attached bpf program.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/include/linux/compiler.h                |   4 +
 .../selftests/bpf/prog_tests/uprobe_syscall.c | 163 ++++++++++++++++++
 .../selftests/bpf/progs/uprobe_syscall.c      |  15 ++
 3 files changed, 182 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
 create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c

diff --git a/tools/include/linux/compiler.h b/tools/include/linux/compiler.h
index 8a63a9913495..6f7f22ac9da5 100644
--- a/tools/include/linux/compiler.h
+++ b/tools/include/linux/compiler.h
@@ -62,6 +62,10 @@
 #define __nocf_check __attribute__((nocf_check))
 #endif
 
+#ifndef __naked
+#define __naked __attribute__((__naked__))
+#endif
+
 /* Are two types/vars the same type (ignoring qualifiers)? */
 #ifndef __same_type
 # define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
new file mode 100644
index 000000000000..14b1cf7a53b4
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+
+#ifdef __x86_64__
+
+#include <unistd.h>
+#include <asm/ptrace.h>
+#include <linux/compiler.h>
+#include "uprobe_syscall.skel.h"
+
+__naked unsigned long uprobe_syscall_arch_test(void)
+{
+	asm volatile (
+		"movq $0xdeadbeef, %rax\n"
+		"ret\n"
+	);
+}
+
+__naked void uprobe_syscall_arch(struct pt_regs *before, struct pt_regs *after)
+{
+	asm volatile (
+		"movq %r15,   0(%rdi)\n"
+		"movq %r14,   8(%rdi)\n"
+		"movq %r13,  16(%rdi)\n"
+		"movq %r12,  24(%rdi)\n"
+		"movq %rbp,  32(%rdi)\n"
+		"movq %rbx,  40(%rdi)\n"
+		"movq %r11,  48(%rdi)\n"
+		"movq %r10,  56(%rdi)\n"
+		"movq  %r9,  64(%rdi)\n"
+		"movq  %r8,  72(%rdi)\n"
+		"movq %rax,  80(%rdi)\n"
+		"movq %rcx,  88(%rdi)\n"
+		"movq %rdx,  96(%rdi)\n"
+		"movq %rsi, 104(%rdi)\n"
+		"movq %rdi, 112(%rdi)\n"
+		"movq   $0, 120(%rdi)\n" /* orig_rax */
+		"movq   $0, 128(%rdi)\n" /* rip      */
+		"movq   $0, 136(%rdi)\n" /* cs       */
+		"pushf\n"
+		"pop %rax\n"
+		"movq %rax, 144(%rdi)\n" /* eflags   */
+		"movq %rsp, 152(%rdi)\n" /* rsp      */
+		"movq   $0, 160(%rdi)\n" /* ss       */
+
+		/* save 2nd argument */
+		"pushq %rsi\n"
+		"call uprobe_syscall_arch_test\n"
+
+		/* save  return value and load 2nd argument pointer to rax */
+		"pushq %rax\n"
+		"movq 8(%rsp), %rax\n"
+
+		"movq %r15,   0(%rax)\n"
+		"movq %r14,   8(%rax)\n"
+		"movq %r13,  16(%rax)\n"
+		"movq %r12,  24(%rax)\n"
+		"movq %rbp,  32(%rax)\n"
+		"movq %rbx,  40(%rax)\n"
+		"movq %r11,  48(%rax)\n"
+		"movq %r10,  56(%rax)\n"
+		"movq  %r9,  64(%rax)\n"
+		"movq  %r8,  72(%rax)\n"
+		"movq %rcx,  88(%rax)\n"
+		"movq %rdx,  96(%rax)\n"
+		"movq %rsi, 104(%rax)\n"
+		"movq %rdi, 112(%rax)\n"
+		"movq   $0, 120(%rax)\n" /* orig_rax */
+		"movq   $0, 128(%rax)\n" /* rip      */
+		"movq   $0, 136(%rax)\n" /* cs       */
+
+		/* restore return value and 2nd argument */
+		"pop %rax\n"
+		"pop %rsi\n"
+
+		"movq %rax,  80(%rsi)\n"
+
+		"pushf\n"
+		"pop %rax\n"
+
+		"movq %rax, 144(%rsi)\n" /* eflags   */
+		"movq %rsp, 152(%rsi)\n" /* rsp      */
+		"movq   $0, 160(%rsi)\n" /* ss       */
+		"ret\n"
+);
+}
+
+static void test_uretprobe_regs_equal(void)
+{
+	struct uprobe_syscall *skel = NULL;
+	struct pt_regs before = {}, after = {};
+	unsigned long *pb = (unsigned long *) &before;
+	unsigned long *pa = (unsigned long *) &after;
+	unsigned long *pp;
+	unsigned int i, cnt;
+	int err;
+
+	skel = uprobe_syscall__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "uprobe_syscall__open_and_load"))
+		goto cleanup;
+
+	err = uprobe_syscall__attach(skel);
+	if (!ASSERT_OK(err, "uprobe_syscall__attach"))
+		goto cleanup;
+
+	uprobe_syscall_arch(&before, &after);
+
+	pp = (unsigned long *) &skel->bss->regs;
+	cnt = sizeof(before)/sizeof(*pb);
+
+	for (i = 0; i < cnt; i++) {
+		unsigned int offset = i * sizeof(unsigned long);
+
+		/*
+		 * Check register before and after uprobe_syscall_arch_test call
+		 * that triggers the uretprobe.
+		 */
+		switch (offset) {
+		case offsetof(struct pt_regs, rax):
+			ASSERT_EQ(pa[i], 0xdeadbeef, "return value");
+			break;
+		default:
+			if (!ASSERT_EQ(pb[i], pa[i], "register before-after value check"))
+				fprintf(stdout, "failed register offset %u\n", offset);
+		}
+
+		/*
+		 * Check register seen from bpf program and register after
+		 * uprobe_syscall_arch_test call
+		 */
+		switch (offset) {
+		/*
+		 * These values will be different (not set in uprobe_syscall_arch),
+		 * we don't care.
+		 */
+		case offsetof(struct pt_regs, orig_rax):
+		case offsetof(struct pt_regs, rip):
+		case offsetof(struct pt_regs, cs):
+		case offsetof(struct pt_regs, rsp):
+		case offsetof(struct pt_regs, ss):
+			break;
+		default:
+			if (!ASSERT_EQ(pp[i], pa[i], "register prog-after value check"))
+				fprintf(stdout, "failed register offset %u\n", offset);
+		}
+	}
+
+cleanup:
+	uprobe_syscall__destroy(skel);
+}
+#else
+static void test_uretprobe_regs_equal(void)
+{
+	test__skip();
+}
+#endif
+
+void test_uprobe_syscall(void)
+{
+	if (test__start_subtest("uretprobe_regs_equal"))
+		test_uretprobe_regs_equal();
+}
diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall.c b/tools/testing/selftests/bpf/progs/uprobe_syscall.c
new file mode 100644
index 000000000000..3b7e0a63f659
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/uprobe_syscall.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <string.h>
+
+struct pt_regs regs;
+
+char _license[] SEC("license") = "GPL";
+
+SEC("uretprobe//proc/self/exe:uprobe_syscall_arch_test")
+int uretprobe(struct pt_regs *ctx)
+{
+	__builtin_memcpy(&regs, ctx, sizeof(regs));
+	return 0;
+}
-- 
2.44.0

[PATCH bpf-next 3/3] selftests/bpf: Add uretprobe syscall test for regs changes

[Jiri Olsa]

Adding test that creates uprobe consumer on uretprobe which changes some
of the registers. Making sure the changed registers are propagated to the
user space when the ureptobe syscall trampoline is used on x86_64.

To be able to do this, adding support to bpf_testmod to create uprobe via
new attribute file:
  /sys/kernel/bpf_testmod_uprobe

This file is expecting file offset and creates related uprobe on current
process exe file and removes existing uprobe if offset is 0. The can be
only single uprobe at any time.

The uprobe has specific consumer that changes registers used in ureprobe
syscall trampoline and which are later checked in the test.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 119 +++++++++++++++++-
 .../selftests/bpf/prog_tests/uprobe_syscall.c |  67 ++++++++++
 2 files changed, 185 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 39ad96a18123..163b6cc64b4f 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -10,6 +10,7 @@
 #include <linux/percpu-defs.h>
 #include <linux/sysfs.h>
 #include <linux/tracepoint.h>
+#include <linux/namei.h>
 #include "bpf_testmod.h"
 #include "bpf_testmod_kfunc.h"
 
@@ -343,6 +344,118 @@ static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
 	.write = bpf_testmod_test_write,
 };
 
+/* bpf_testmod_uprobe sysfs attribute is so far enabled for x86_64 only,
+ * please see test_uretprobe_regs_change test  */
+#ifdef __x86_64__
+
+static int
+uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,
+		   struct pt_regs *regs)
+
+{
+	regs->ax  = 0x12345678deadbeef;
+	regs->cx  = 0x87654321feebdaed;
+	regs->r11 = (u64) -1;
+	return true;
+}
+
+struct testmod_uprobe {
+	struct path path;
+	loff_t offset;
+	struct uprobe_consumer consumer;
+};
+
+static DEFINE_MUTEX(testmod_uprobe_mutex);
+
+static struct testmod_uprobe uprobe = {
+	.consumer.ret_handler = uprobe_ret_handler,
+};
+
+static int testmod_register_uprobe(loff_t offset)
+{
+	int err = -EBUSY;
+
+	if (uprobe.offset)
+		return -EBUSY;
+
+	mutex_lock(&testmod_uprobe_mutex);
+
+	if (uprobe.offset)
+		goto out;
+
+	err = kern_path("/proc/self/exe", LOOKUP_FOLLOW, &uprobe.path);
+	if (err)
+		goto out;
+
+	err = uprobe_register_refctr(d_real_inode(uprobe.path.dentry),
+				     offset, 0, &uprobe.consumer);
+	if (err)
+		path_put(&uprobe.path);
+	else
+		uprobe.offset = offset;
+
+out:
+	mutex_unlock(&testmod_uprobe_mutex);
+	return err;
+}
+
+static void testmod_unregister_uprobe(void)
+{
+	mutex_lock(&testmod_uprobe_mutex);
+
+	if (uprobe.offset) {
+		uprobe_unregister(d_real_inode(uprobe.path.dentry),
+				  uprobe.offset, &uprobe.consumer);
+		uprobe.offset = 0;
+	}
+
+	mutex_unlock(&testmod_uprobe_mutex);
+}
+
+static ssize_t
+bpf_testmod_uprobe_write(struct file *file, struct kobject *kobj,
+			 struct bin_attribute *bin_attr,
+			 char *buf, loff_t off, size_t len)
+{
+	unsigned long offset;
+	int err;
+
+	if (kstrtoul(buf, 0, &offset))
+		return -EINVAL;
+
+	if (offset)
+		err = testmod_register_uprobe(offset);
+	else
+		testmod_unregister_uprobe();
+
+	return err ?: strlen(buf);
+}
+
+static struct bin_attribute bin_attr_bpf_testmod_uprobe_file __ro_after_init = {
+	.attr = { .name = "bpf_testmod_uprobe", .mode = 0666, },
+	.write = bpf_testmod_uprobe_write,
+};
+
+static int register_bpf_testmod_uprobe(void)
+{
+	return sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_uprobe_file);
+}
+
+static void unregister_bpf_testmod_uprobe(void)
+{
+	testmod_unregister_uprobe();
+	sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_uprobe_file);
+}
+
+#else
+static int register_bpf_testmod_uprobe(void)
+{
+	return 0;
+}
+
+static void unregister_bpf_testmod_uprobe(void) { }
+#endif
+
 BTF_KFUNCS_START(bpf_testmod_common_kfunc_ids)
 BTF_ID_FLAGS(func, bpf_iter_testmod_seq_new, KF_ITER_NEW)
 BTF_ID_FLAGS(func, bpf_iter_testmod_seq_next, KF_ITER_NEXT | KF_RET_NULL)
@@ -650,7 +763,10 @@ static int bpf_testmod_init(void)
 		return ret;
 	if (bpf_fentry_test1(0) < 0)
 		return -EINVAL;
-	return sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+	ret = sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+	if (ret < 0)
+		return ret;
+	return register_bpf_testmod_uprobe();
 }
 
 static void bpf_testmod_exit(void)
@@ -664,6 +780,7 @@ static void bpf_testmod_exit(void)
 		msleep(20);
 
 	sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+	unregister_bpf_testmod_uprobe();
 }
 
 module_init(bpf_testmod_init);
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
index 14b1cf7a53b4..7042da10bb69 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -149,15 +149,82 @@ static void test_uretprobe_regs_equal(void)
 cleanup:
 	uprobe_syscall__destroy(skel);
 }
+
+#define BPF_TESTMOD_UPROBE_TEST_FILE "/sys/kernel/bpf_testmod_uprobe"
+
+static int write_bpf_testmod_uprobe(unsigned long offset)
+{
+	size_t n, ret;
+	char buf[30];
+	int fd;
+
+	n = sprintf(buf, "%lu", offset);
+
+	fd = open(BPF_TESTMOD_UPROBE_TEST_FILE, O_WRONLY);
+	if (fd < 0)
+		return -errno;
+
+	ret = write(fd, buf, n);
+	close(fd);
+	return ret != n ? (int) ret : 0;
+}
+
+static void test_uretprobe_regs_change(void)
+{
+	struct pt_regs before = {}, after = {};
+	unsigned long *pb = (unsigned long *) &before;
+	unsigned long *pa = (unsigned long *) &after;
+	unsigned long cnt = sizeof(before)/sizeof(*pb);
+	unsigned int i, err, offset;
+
+	offset = get_uprobe_offset(uprobe_syscall_arch_test);
+
+	err = write_bpf_testmod_uprobe(offset);
+	if (!ASSERT_OK(err, "register_uprobe"))
+		return;
+
+	uprobe_syscall_arch(&before, &after);
+
+	err = write_bpf_testmod_uprobe(0);
+	if (!ASSERT_OK(err, "unregister_uprobe"))
+		return;
+
+	for (i = 0; i < cnt; i++) {
+		unsigned int offset = i * sizeof(unsigned long);
+
+		switch (offset) {
+		case offsetof(struct pt_regs, rax):
+			ASSERT_EQ(pa[i], 0x12345678deadbeef, "rax");
+			break;
+		case offsetof(struct pt_regs, rcx):
+			ASSERT_EQ(pa[i], 0x87654321feebdaed, "rcx");
+			break;
+		case offsetof(struct pt_regs, r11):
+			ASSERT_EQ(pa[i], (__u64) -1, "r11");
+			break;
+		default:
+			if (!ASSERT_EQ(pa[i], pb[i], "register before-after value check"))
+				fprintf(stdout, "failed register offset %u\n", offset);
+		}
+	}
+}
+
 #else
 static void test_uretprobe_regs_equal(void)
 {
 	test__skip();
 }
+
+static void test_uretprobe_regs_change(void)
+{
+	test__skip();
+}
 #endif
 
 void test_uprobe_syscall(void)
 {
 	if (test__start_subtest("uretprobe_regs_equal"))
 		test_uretprobe_regs_equal();
+	if (test__start_subtest("uretprobe_regs_change"))
+		test_uretprobe_regs_change();
 }
-- 
2.44.0

Re: [PATCH bpf-next 1/3] uprobe: Add uretprobe syscall to speed up return probe

[Oleg Nesterov]

On 03/27, Jiri Olsa wrote:
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  arch/x86/entry/syscalls/syscall_64.tbl |  1 +
>  arch/x86/kernel/uprobes.c              | 83 ++++++++++++++++++++++++++
>  include/linux/syscalls.h               |  2 +
>  include/linux/uprobes.h                |  2 +
>  include/uapi/asm-generic/unistd.h      |  5 +-
>  kernel/events/uprobes.c                | 18 ++++--
>  kernel/sys_ni.c                        |  2 +
>  7 files changed, 108 insertions(+), 5 deletions(-)

Just in case,

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH bpf-next 1/3] uprobe: Add uretprobe syscall to speed up return probe

[Andrii Nakryiko]

On Wed, Mar 27, 2024 at 3:20 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding uretprobe syscall instead of trap to speed up return probe.
>
> At the moment the uretprobe setup/path is:
>
>   - install entry uprobe
>
>   - when the uprobe is hit, it overwrites probed function's return address
>     on stack with address of the trampoline that contains breakpoint
>     instruction
>
>   - the breakpoint trap code handles the uretprobe consumers execution and
>     jumps back to original return address
>
> This patch replaces the above trampoline's breakpoint instruction with new
> ureprobe syscall call. This syscall does exactly the same job as the trap
> with some more extra work:
>
>   - syscall trampoline must save original value for rax/r11/rcx registers
>     on stack - rax is set to syscall number and r11/rcx are changed and
>     used by syscall instruction
>
>   - the syscall code reads the original values of those registers and
>     restore those values in task's pt_regs area
>
> Even with the extra registers handling code the having uretprobes handled
> by syscalls shows speed improvement.
>
>   On Intel (11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz)
>
>   current:
>
>     base           :   15.888 ± 0.033M/s
>     uprobe-nop     :    3.016 ± 0.000M/s
>     uprobe-push    :    2.832 ± 0.005M/s
>     uprobe-ret     :    1.104 ± 0.000M/s
>     uretprobe-nop  :    1.487 ± 0.000M/s
>     uretprobe-push :    1.456 ± 0.000M/s
>     uretprobe-ret  :    0.816 ± 0.001M/s
>
>   with the fix:
>
>     base           :   15.116 ± 0.045M/s
>     uprobe-nop     :    3.001 ± 0.045M/s
>     uprobe-push    :    2.831 ± 0.004M/s
>     uprobe-ret     :    1.102 ± 0.001M/s
>     uretprobe-nop  :    1.969 ± 0.001M/s  < 32% speedup
>     uretprobe-push :    1.905 ± 0.004M/s  < 30% speedup
>     uretprobe-ret  :    0.933 ± 0.002M/s  < 14% speedup
>
>   On Amd (AMD Ryzen 7 5700U)
>
>   current:
>
>     base           :    5.105 ± 0.003M/s
>     uprobe-nop     :    1.552 ± 0.002M/s
>     uprobe-push    :    1.408 ± 0.003M/s
>     uprobe-ret     :    0.827 ± 0.001M/s
>     uretprobe-nop  :    0.779 ± 0.001M/s
>     uretprobe-push :    0.750 ± 0.001M/s
>     uretprobe-ret  :    0.539 ± 0.001M/s
>
>   with the fix:
>
>     base           :    5.119 ± 0.002M/s
>     uprobe-nop     :    1.523 ± 0.003M/s
>     uprobe-push    :    1.384 ± 0.003M/s
>     uprobe-ret     :    0.826 ± 0.002M/s
>     uretprobe-nop  :    0.866 ± 0.002M/s  < 11% speedup
>     uretprobe-push :    0.826 ± 0.002M/s  < 10% speedup
>     uretprobe-ret  :    0.581 ± 0.001M/s  <  7% speedup
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  arch/x86/entry/syscalls/syscall_64.tbl |  1 +
>  arch/x86/kernel/uprobes.c              | 83 ++++++++++++++++++++++++++
>  include/linux/syscalls.h               |  2 +
>  include/linux/uprobes.h                |  2 +
>  include/uapi/asm-generic/unistd.h      |  5 +-
>  kernel/events/uprobes.c                | 18 ++++--
>  kernel/sys_ni.c                        |  2 +
>  7 files changed, 108 insertions(+), 5 deletions(-)
>

Great work and results, thanks!

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

[...]

Re: [PATCH bpf-next 2/3] selftests/bpf: Add uretprobe syscall test for regs integrity

[Andrii Nakryiko]

On Wed, Mar 27, 2024 at 3:21 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Add uretprobe syscall test that compares register values before
> and after the uretprobe is hit. It also compares the register
> values seen from attached bpf program.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/include/linux/compiler.h                |   4 +
>  .../selftests/bpf/prog_tests/uprobe_syscall.c | 163 ++++++++++++++++++
>  .../selftests/bpf/progs/uprobe_syscall.c      |  15 ++
>  3 files changed, 182 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
>  create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c

[...]

> +__naked unsigned long uprobe_syscall_arch_test(void)
> +{
> +       asm volatile (
> +               "movq $0xdeadbeef, %rax\n"
> +               "ret\n"
> +       );
> +}
> +
> +__naked void uprobe_syscall_arch(struct pt_regs *before, struct pt_regs *after)

don't you get compiler warnings for using __naked with explicit
function arguments?

> +{
> +       asm volatile (
> +               "movq %r15,   0(%rdi)\n"
> +               "movq %r14,   8(%rdi)\n"

[...]

> +       err = uprobe_syscall__attach(skel);
> +       if (!ASSERT_OK(err, "uprobe_syscall__attach"))
> +               goto cleanup;
> +
> +       uprobe_syscall_arch(&before, &after);

uprobe_syscall_arch() doesn't really do an explicit `syscall
uretprobe`, it should work for int3-based uretprobes as well? Let's
call it something a bit more generic then?

Also, I think patch #1 will go through Masami's trace tree, right? But
we can land selftests into bpf-next even before that, given they
should work for both syscall and interrupt based uretprobes.

> +
> +       pp = (unsigned long *) &skel->bss->regs;
> +       cnt = sizeof(before)/sizeof(*pb);
> +

[...]

Re: [PATCH bpf-next 3/3] selftests/bpf: Add uretprobe syscall test for regs changes

[Andrii Nakryiko]

On Wed, Mar 27, 2024 at 3:21 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test that creates uprobe consumer on uretprobe which changes some
> of the registers. Making sure the changed registers are propagated to the
> user space when the ureptobe syscall trampoline is used on x86_64.
>
> To be able to do this, adding support to bpf_testmod to create uprobe via
> new attribute file:
>   /sys/kernel/bpf_testmod_uprobe
>
> This file is expecting file offset and creates related uprobe on current
> process exe file and removes existing uprobe if offset is 0. The can be
> only single uprobe at any time.
>
> The uprobe has specific consumer that changes registers used in ureprobe
> syscall trampoline and which are later checked in the test.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 119 +++++++++++++++++-
>  .../selftests/bpf/prog_tests/uprobe_syscall.c |  67 ++++++++++
>  2 files changed, 185 insertions(+), 1 deletion(-)
>

LGTM:

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

[...]

>  BTF_KFUNCS_START(bpf_testmod_common_kfunc_ids)
>  BTF_ID_FLAGS(func, bpf_iter_testmod_seq_new, KF_ITER_NEW)
>  BTF_ID_FLAGS(func, bpf_iter_testmod_seq_next, KF_ITER_NEXT | KF_RET_NULL)
> @@ -650,7 +763,10 @@ static int bpf_testmod_init(void)
>                 return ret;
>         if (bpf_fentry_test1(0) < 0)
>                 return -EINVAL;
> -       return sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
> +       ret = sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
> +       if (ret < 0)
> +               return ret;
> +       return register_bpf_testmod_uprobe();

nit: keep using the same pattern to make adding more actions easier in
the future:

ret = register_bpf_testmod_uprobe();
if (ret < 0)
    return ret;
return 0;

>  }
>
>  static void bpf_testmod_exit(void)
> @@ -664,6 +780,7 @@ static void bpf_testmod_exit(void)
>                 msleep(20);
>
>         sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
> +       unregister_bpf_testmod_uprobe();
>  }
>

[...]

Re: [PATCH bpf-next 2/3] selftests/bpf: Add uretprobe syscall test for regs integrity

[Andrii Nakryiko]

On Fri, Mar 29, 2024 at 3:30 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Mar 27, 2024 at 3:21 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Add uretprobe syscall test that compares register values before
> > and after the uretprobe is hit. It also compares the register
> > values seen from attached bpf program.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/include/linux/compiler.h                |   4 +
> >  .../selftests/bpf/prog_tests/uprobe_syscall.c | 163 ++++++++++++++++++
> >  .../selftests/bpf/progs/uprobe_syscall.c      |  15 ++
> >  3 files changed, 182 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c
>
> [...]
>
> > +__naked unsigned long uprobe_syscall_arch_test(void)
> > +{
> > +       asm volatile (
> > +               "movq $0xdeadbeef, %rax\n"
> > +               "ret\n"
> > +       );
> > +}
> > +
> > +__naked void uprobe_syscall_arch(struct pt_regs *before, struct pt_regs *after)
>
> don't you get compiler warnings for using __naked with explicit
> function arguments?
>
> > +{
> > +       asm volatile (
> > +               "movq %r15,   0(%rdi)\n"
> > +               "movq %r14,   8(%rdi)\n"
>
> [...]
>
> > +       err = uprobe_syscall__attach(skel);
> > +       if (!ASSERT_OK(err, "uprobe_syscall__attach"))
> > +               goto cleanup;
> > +
> > +       uprobe_syscall_arch(&before, &after);
>
> uprobe_syscall_arch() doesn't really do an explicit `syscall
> uretprobe`, it should work for int3-based uretprobes as well? Let's
> call it something a bit more generic then?
>
> Also, I think patch #1 will go through Masami's trace tree, right? But
> we can land selftests into bpf-next even before that, given they
> should work for both syscall and interrupt based uretprobes.

Regardless the decision:

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

>
> > +
> > +       pp = (unsigned long *) &skel->bss->regs;
> > +       cnt = sizeof(before)/sizeof(*pb);
> > +
>
> [...]

Re: [PATCH bpf-next 3/3] selftests/bpf: Add uretprobe syscall test for regs changes

[Jiri Olsa]

On Fri, Mar 29, 2024 at 03:34:00PM -0700, Andrii Nakryiko wrote:
> On Wed, Mar 27, 2024 at 3:21 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding test that creates uprobe consumer on uretprobe which changes some
> > of the registers. Making sure the changed registers are propagated to the
> > user space when the ureptobe syscall trampoline is used on x86_64.
> >
> > To be able to do this, adding support to bpf_testmod to create uprobe via
> > new attribute file:
> >   /sys/kernel/bpf_testmod_uprobe
> >
> > This file is expecting file offset and creates related uprobe on current
> > process exe file and removes existing uprobe if offset is 0. The can be
> > only single uprobe at any time.
> >
> > The uprobe has specific consumer that changes registers used in ureprobe
> > syscall trampoline and which are later checked in the test.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 119 +++++++++++++++++-
> >  .../selftests/bpf/prog_tests/uprobe_syscall.c |  67 ++++++++++
> >  2 files changed, 185 insertions(+), 1 deletion(-)
> >
> 
> LGTM:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> [...]
> 
> >  BTF_KFUNCS_START(bpf_testmod_common_kfunc_ids)
> >  BTF_ID_FLAGS(func, bpf_iter_testmod_seq_new, KF_ITER_NEW)
> >  BTF_ID_FLAGS(func, bpf_iter_testmod_seq_next, KF_ITER_NEXT | KF_RET_NULL)
> > @@ -650,7 +763,10 @@ static int bpf_testmod_init(void)
> >                 return ret;
> >         if (bpf_fentry_test1(0) < 0)
> >                 return -EINVAL;
> > -       return sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
> > +       ret = sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
> > +       if (ret < 0)
> > +               return ret;
> > +       return register_bpf_testmod_uprobe();
> 
> nit: keep using the same pattern to make adding more actions easier in
> the future:
> 
> ret = register_bpf_testmod_uprobe();
> if (ret < 0)
>     return ret;
> return 0;

ok, will change 

thanks,
jirka

> 
> >  }
> >
> >  static void bpf_testmod_exit(void)
> > @@ -664,6 +780,7 @@ static void bpf_testmod_exit(void)
> >                 msleep(20);
> >
> >         sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
> > +       unregister_bpf_testmod_uprobe();
> >  }
> >
> 
> [...]

Re: [PATCH bpf-next 2/3] selftests/bpf: Add uretprobe syscall test for regs integrity

[Jiri Olsa]

On Fri, Mar 29, 2024 at 03:30:11PM -0700, Andrii Nakryiko wrote:
> On Wed, Mar 27, 2024 at 3:21 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Add uretprobe syscall test that compares register values before
> > and after the uretprobe is hit. It also compares the register
> > values seen from attached bpf program.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/include/linux/compiler.h                |   4 +
> >  .../selftests/bpf/prog_tests/uprobe_syscall.c | 163 ++++++++++++++++++
> >  .../selftests/bpf/progs/uprobe_syscall.c      |  15 ++
> >  3 files changed, 182 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c
> 
> [...]
> 
> > +__naked unsigned long uprobe_syscall_arch_test(void)
> > +{
> > +       asm volatile (
> > +               "movq $0xdeadbeef, %rax\n"
> > +               "ret\n"
> > +       );
> > +}
> > +
> > +__naked void uprobe_syscall_arch(struct pt_regs *before, struct pt_regs *after)
> 
> don't you get compiler warnings for using __naked with explicit
> function arguments?

nope, both gcc and clang are silent

> 
> > +{
> > +       asm volatile (
> > +               "movq %r15,   0(%rdi)\n"
> > +               "movq %r14,   8(%rdi)\n"
> 
> [...]
> 
> > +       err = uprobe_syscall__attach(skel);
> > +       if (!ASSERT_OK(err, "uprobe_syscall__attach"))
> > +               goto cleanup;
> > +
> > +       uprobe_syscall_arch(&before, &after);
> 
> uprobe_syscall_arch() doesn't really do an explicit `syscall
> uretprobe`, it should work for int3-based uretprobes as well? Let's
> call it something a bit more generic then?

ok, how about

  uprobe_syscall_arch -> uretprobe_regs
  uprobe_syscall_arch_test -> uretprobe_regs_trigger

> 
> Also, I think patch #1 will go through Masami's trace tree, right? But
> we can land selftests into bpf-next even before that, given they
> should work for both syscall and interrupt based uretprobes.

hm, not sure.. I did not originally cc Masami/Steven :-\  adding now

Masami, could patch 1 go through:

  https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
  probes/for-next

thanks,
jirka

Re: [PATCH bpf-next 2/3] selftests/bpf: Add uretprobe syscall test for regs integrity

[Masami Hiramatsu]

Hi Jiri,

On Sun, 31 Mar 2024 21:37:03 +0200
Jiri Olsa <olsajiri@gmail.com> wrote:

> On Fri, Mar 29, 2024 at 03:30:11PM -0700, Andrii Nakryiko wrote:
> > On Wed, Mar 27, 2024 at 3:21 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Add uretprobe syscall test that compares register values before
> > > and after the uretprobe is hit. It also compares the register
> > > values seen from attached bpf program.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/include/linux/compiler.h                |   4 +
> > >  .../selftests/bpf/prog_tests/uprobe_syscall.c | 163 ++++++++++++++++++
> > >  .../selftests/bpf/progs/uprobe_syscall.c      |  15 ++
> > >  3 files changed, 182 insertions(+)
> > >  create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > >  create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c
> > 
> > [...]
> > 
> > > +__naked unsigned long uprobe_syscall_arch_test(void)
> > > +{
> > > +       asm volatile (
> > > +               "movq $0xdeadbeef, %rax\n"
> > > +               "ret\n"
> > > +       );
> > > +}
> > > +
> > > +__naked void uprobe_syscall_arch(struct pt_regs *before, struct pt_regs *after)
> > 
> > don't you get compiler warnings for using __naked with explicit
> > function arguments?
> 
> nope, both gcc and clang are silent
> 
> > 
> > > +{
> > > +       asm volatile (
> > > +               "movq %r15,   0(%rdi)\n"
> > > +               "movq %r14,   8(%rdi)\n"
> > 
> > [...]
> > 
> > > +       err = uprobe_syscall__attach(skel);
> > > +       if (!ASSERT_OK(err, "uprobe_syscall__attach"))
> > > +               goto cleanup;
> > > +
> > > +       uprobe_syscall_arch(&before, &after);
> > 
> > uprobe_syscall_arch() doesn't really do an explicit `syscall
> > uretprobe`, it should work for int3-based uretprobes as well? Let's
> > call it something a bit more generic then?
> 
> ok, how about
> 
>   uprobe_syscall_arch -> uretprobe_regs
>   uprobe_syscall_arch_test -> uretprobe_regs_trigger
> 
> > 
> > Also, I think patch #1 will go through Masami's trace tree, right? But
> > we can land selftests into bpf-next even before that, given they
> > should work for both syscall and interrupt based uretprobes.
> 
> hm, not sure.. I did not originally cc Masami/Steven :-\  adding now

Would you mean this patch?

https://lore.kernel.org/all/20240327102036.543283-2-jolsa@kernel.org/

It seems you don't Cc/To me nor linux-kernel-trace ML.

> 
> Masami, could patch 1 go through:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
>   probes/for-next

Could you resend it to linux-kernel-trace ML?  (only the first one?)

Thank you,

> 
> thanks,
> jirka


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>