On Tue, Sep 28, 2021 at 8:46 AM Yonghong Song [off-list ref] wrote:

On 9/27/21 10:11 PM, Andrii Nakryiko wrote:

quoted

On Mon, Sep 27, 2021 at 10:05 AM Yonghong Song [off-list ref] wrote:

quoted

clang build kernel failed the selftest probe_user.
   $ ./test_progs -t probe_user
   $ ...
   $ test_probe_user:PASS:get_kprobe_res 0 nsec
   $ test_probe_user:FAIL:check_kprobe_res wrong kprobe res from probe read: 0.0.0.0:0
   $ #94 probe_user:FAIL

The test attached to kernel function __sys_connect(). In net/socket.c, we have
   int __sys_connect(int fd, struct sockaddr __user *uservaddr, int addrlen)
   {
         ......
   }
   ...
   SYSCALL_DEFINE3(connect, int, fd, struct sockaddr __user *, uservaddr,
                   int, addrlen)
   {
         return __sys_connect(fd, uservaddr, addrlen);
   }

The gcc compiler (8.5.0) does not inline __sys_connect() in syscall entry
function. But latest clang trunk did the inlining. So the bpf program
is not triggered.

To make the test more reliable, let us kprobe the syscall entry function
instead. But x86_64, arm64 and s390 has syscall wrapper and they have
to be handled specially. I also changed the test to use vmlinux.h and CORE
to accommodate relocatable pt_regs structure, similar to
samples/bpf/test_probe_write_user_kern.c.

Signed-off-by: Yonghong Song <redacted>
---
  .../selftests/bpf/prog_tests/probe_user.c     |  4 +--
  .../selftests/bpf/progs/test_probe_user.c     | 30 +++++++++++++++----
  2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/probe_user.c b/tools/testing/selftests/bpf/prog_tests/probe_user.c
index 95bd12097358..52fe157e2a90 100644
--- a/tools/testing/selftests/bpf/prog_tests/probe_user.c
+++ b/tools/testing/selftests/bpf/prog_tests/probe_user.c

@@ -3,7 +3,7 @@

  void test_probe_user(void)
  {
-       const char *prog_name = "kprobe/__sys_connect";
+       const char *prog_name = "handle_sys_connect";
         const char *obj_file = "./test_probe_user.o";
         DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts, );
         int err, results_map_fd, sock_fd, duration = 0;

@@ -18,7 +18,7 @@ void test_probe_user(void)
         if (!ASSERT_OK_PTR(obj, "obj_open_file"))
                 return;

-       kprobe_prog = bpf_object__find_program_by_title(obj, prog_name);
+       kprobe_prog = bpf_object__find_program_by_name(obj, prog_name);
         if (CHECK(!kprobe_prog, "find_probe",
                   "prog '%s' not found\n", prog_name))
                 goto cleanup;

diff --git a/tools/testing/selftests/bpf/progs/test_probe_user.c b/tools/testing/selftests/bpf/progs/test_probe_user.c
index 89b3532ccc75..9b3ddbf6289d 100644
--- a/tools/testing/selftests/bpf/progs/test_probe_user.c
+++ b/tools/testing/selftests/bpf/progs/test_probe_user.c

@@ -1,21 +1,39 @@
  // SPDX-License-Identifier: GPL-2.0

-#include <linux/ptrace.h>
-#include <linux/bpf.h>
-
-#include <netinet/in.h>
+#include "vmlinux.h"

  #include <bpf/bpf_helpers.h>
+#include <bpf/bpf_core_read.h>
  #include <bpf/bpf_tracing.h>

+#if defined(__TARGET_ARCH_x86)

I mangled this check (locally) to test the #else case, and it failed
compilation:

progs/test_probe_user.c:24:15: error: expected ')'
SEC("kprobe/" SYS_PREFIX "sys_connect")

adding #define SYS_PREFIX "" fixes the issue.

But I'm also curious:

1. do we need all this arch-specific logic? maybe we can find some
other and more stable interface to attach to? e.g., would
move_addr_to_kernel() work? it's a global function, so it shouldn't be
inlined, right? Or did you intend to also demo the use of
PT_REGS_PARM1_CORE() with this?

The following are move_add_to_kernel() call usages:

fs/io_uring.c:  return move_addr_to_kernel(conn->addr, conn->addr_len,
&io->address);
fs/io_uring.c:          ret = move_addr_to_kernel(req->connect.addr,
include/linux/socket.h:extern int move_addr_to_kernel(void __user
*uaddr, int ulen, struct sockaddr_storage *kaddr);
net/compat.c:                   err =
move_addr_to_kernel(compat_ptr(msg.msg_name),
net/socket.c: * move_addr_to_kernel     -       copy a socket address
into kernel space
net/socket.c:int move_addr_to_kernel(void __user *uaddr, int ulen,
struct sockaddr_storage *kaddr)
net/socket.c:           err = move_addr_to_kernel(umyaddr, addrlen,
&address);
net/socket.c:           ret = move_addr_to_kernel(uservaddr, addrlen,
&address);
net/socket.c:           err = move_addr_to_kernel(addr, addr_len, &address);
net/socket.c:                   err = move_addr_to_kernel(msg.msg_name,

inlining could happen within net/socket.c. Another two use cases are
fs/io_uring.c and net/compat.c. Using compat syscall may be too complex,
I assume. Using io_uring with bpf selftest may be a choice but I think
it makes thing too complicated.

io_uring for this would be overkill

More importantly, assuming in the future the kernel may be compiled
with LTO, move_addr_to_kernel() might be inlined into fs/io_uring.c.
So that is the main reason I am using syscall entry point directly.

Yep, makes sense.

You are right, vmlinux.h is generated from the very kernel we are
testing so we don't need CORE. I added CORE similar to
samples/bpf/test_probe_write_user_kern.c, but it is not really
needed here.

quoted

2. global .rodata variable isn't really necessary. Static one would
work just fine and would be eliminated by the compiler at compilation
time. Or equivalently just doing #define SYS_WRAPPER 1 for all cases
but #else would work as well. I don't mind global var, but I'm just
curious if you explicitly wanted to test .rodata global variable in
this case?

Good point. This test is not to test .rodata variable. So using macro
to differentiate different cases is actually better.

Ok, sounds good.

quoted

quoted

+volatile const int syscall_wrapper = 1;
+#define SYS_PREFIX "__x64_"
+#elif defined(__TARGET_ARCH_s390)
+volatile const int syscall_wrapper = 1;
+#define SYS_PREFIX "__s390x_"
+#elif defined(__TARGET_ARCH_arm64)
+volatile const int syscall_wrapper = 1;
+#define SYS_PREFIX "__arm64_"
+#else
+volatile const int syscall_wrapper = 0;
+#endif
+
  static struct sockaddr_in old;

-SEC("kprobe/__sys_connect")
+SEC("kprobe/" SYS_PREFIX "sys_connect")
  int BPF_KPROBE(handle_sys_connect)
  {
-       void *ptr = (void *)PT_REGS_PARM2(ctx);
+       void *ptr;
         struct sockaddr_in new;

+       if (syscall_wrapper == 0) {
+               ptr = (void *)PT_REGS_PARM2_CORE(ctx);
+       } else {
+               struct pt_regs *real_regs = (struct pt_regs *)PT_REGS_PARM1_CORE(ctx);
+               ptr = (void *)PT_REGS_PARM2_CORE(real_regs);
+       }
+
         bpf_probe_read_user(&old, sizeof(old), ptr);
         __builtin_memset(&new, 0xab, sizeof(new));
         bpf_probe_write_user(ptr, &new, sizeof(new));
--
2.30.2