On Tue, Sep 28, 2021 at 8:46 AM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 9/27/21 10:11 PM, Andrii Nakryiko wrote:
> > On Mon, Sep 27, 2021 at 10:05 AM Yonghong Song <yhs@xxxxxx> wrote:
> >>
> >> 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 <yhs@xxxxxx>
> >> ---
> >> .../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.
>
> >
> > 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.
>
> >
> >
> >> +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
> >>
