On Tue, Jan 17, 2023 at 1:52 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote:
>
> On Mon, 2023-01-16 at 23:37 +0100, Ilya Leoshkevich wrote:
> > On Mon, 2023-01-16 at 23:09 +0100, Ilya Leoshkevich wrote:
> > > On Fri, 2023-01-13 at 00:33 -0800, Andrii Nakryiko wrote:
> > > > This patch set fixes and extends libbpf's bpf_tracing.h support
> > > > for
> > > > tracing
> > > > arguments of kprobes/uprobes, and syscall as a special case.
> > > >
> > > > Depending on the architecture, anywhere between 3 and 8 arguments
> > > > can
> > > > be
> > > > passed to a function in registers (so relevant to kprobes and
> > > > uprobes), but
> > > > before this patch set libbpf's macros in bpf_tracing.h only
> > > > supported
> > > > up to
> > > > 5 arguments, which is limiting in practice. This patch set
> > > > extends
> > > > bpf_tracing.h to support up to 8 arguments, if architecture
> > > > allows.
> > > > This
> > > > includes explicit PT_REGS_PARMx() macro family, as well as
> > > > BPF_KPROBE() macro.
> > > >
> > > > Now, with tracing syscall arguments situation is sometimes quite
> > > > different.
> > > > For a lot of architectures syscall argument passing through
> > > > registers
> > > > differs
> > > > from function call sequence at least a little. For i386 it
> > > > differs
> > > > *a
> > > > lot*.
> > > > This patch set addresses this issue across all currently
> > > > supported
> > > > architectures and hopefully fixes existing issues. syscall(2)
> > > > manpage
> > > > defines
> > > > that either 6 or 7 arguments can be supported, depending on
> > > > architecture, so
> > > > libbpf defines 6 or 7 registers per architecture to be used to
> > > > fetch
> > > > syscall
> > > > arguments.
> > > >
> > > > Also, BPF_UPROBE and BPF_URETPROBE are introduced as part of this
> > > > patch set.
> > > > They are aliases for BPF_KPROBE and BPF_KRETPROBE (as mechanics
> > > > of
> > > > argument
> > > > fetching of kernel functions and user-space functions are
> > > > identical),
> > > > but it
> > > > allows BPF users to have less confusing BPF-side code when
> > > > working
> > > > with
> > > > uprobes.
> > > >
> > > > For both sets of changes selftests are extended to test these new
> > > > register
> > > > definitions to architecture-defined limits. Unfortunately I don't
> > > > have ability
> > > > to test it on all architectures, and BPF CI only tests 3
> > > > architecture
> > > > (x86-64,
> > > > arm64, and s390x), so it would be greatly appreciated if CC'ed
> > > > people
> > > > can help
> > > > review and test changes on architectures they are familiar with
> > > > (and
> > > > maybe
> > > > have direct access to for testing). Thank you.
> > > >
> > > > Cc: Alan Maguire <alan.maguire@xxxxxxxxxx>
> > > > Cc: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> > > > Cc: Pu Lehui <pulehui@xxxxxxxxxx>
> > > > Cc: Hengqi Chen <hengqi.chen@xxxxxxxxx>
> > > > Cc: Vladimir Isaev <isaev@xxxxxxxxxxxx>
> > > > Cc: Björn Töpel <bjorn@xxxxxxxxxx>
> > > > Cc: Kenta Tada <Kenta.Tada@xxxxxxxx>
> > > > Cc: Florent Revest <revest@xxxxxxxxxxxx>
> > > >
> > > > Andrii Nakryiko (25):
> > > > libbpf: add support for fetching up to 8 arguments in kprobes
> > > > libbpf: add 6th argument support for x86-64 in bpf_tracing.h
> > > > libbpf: fix arm and arm64 specs in bpf_tracing.h
> > > > libbpf: complete mips spec in bpf_tracing.h
> > > > libbpf: complete powerpc spec in bpf_tracing.h
> > > > libbpf: complete sparc spec in bpf_tracing.h
> > > > libbpf: complete riscv arch spec in bpf_tracing.h
> > > > libbpf: fix and complete ARC spec in bpf_tracing.h
> > > > libbpf: complete LoongArch (loongarch) spec in bpf_tracing.h
> > > > libbpf: add BPF_UPROBE and BPF_URETPROBE macro aliases
> > > > selftests/bpf: validate arch-specific argument registers limits
> > > > libbpf: improve syscall tracing support in bpf_tracing.h
> > > > libbpf: define x86-64 syscall regs spec in bpf_tracing.h
> > > > libbpf: define i386 syscall regs spec in bpf_tracing.h
> > > > libbpf: define s390x syscall regs spec in bpf_tracing.h
> > > > libbpf: define arm syscall regs spec in bpf_tracing.h
> > > > libbpf: define arm64 syscall regs spec in bpf_tracing.h
> > > > libbpf: define mips syscall regs spec in bpf_tracing.h
> > > > libbpf: define powerpc syscall regs spec in bpf_tracing.h
> > > > libbpf: define sparc syscall regs spec in bpf_tracing.h
> > > > libbpf: define riscv syscall regs spec in bpf_tracing.h
> > > > libbpf: define arc syscall regs spec in bpf_tracing.h
> > > > libbpf: define loongarch syscall regs spec in bpf_tracing.h
> > > > selftests/bpf: add 6-argument syscall tracing test
> > > > libbpf: clean up now not needed __PT_PARM{1-6}_SYSCALL_REG
> > > > defaults
> > > >
> > > > tools/lib/bpf/bpf_tracing.h | 301
> > > > +++++++++++++++-
> > > > --
> > > > .../bpf/prog_tests/test_bpf_syscall_macro.c | 18 +-
> > > > .../bpf/prog_tests/uprobe_autoattach.c | 33 +-
> > > > tools/testing/selftests/bpf/progs/bpf_misc.h | 25 ++
> > > > .../selftests/bpf/progs/bpf_syscall_macro.c | 26 ++
> > > > .../bpf/progs/test_uprobe_autoattach.c | 48 ++-
> > > > 6 files changed, 405 insertions(+), 46 deletions(-)
> > > >
> > >
> > > With the following fixup for 24/25:
>
> [...]
>
> > > Tested-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx> # s390x
> >
> > While the above fixup works, I realized that it's ugly. It's better
> > to
> > admit that mmap and old_mmap are different syscalls and create a
> > different probe, even if it means duplicating pid filtering code:
>
> [...]
>
> Sorry, I'm being dense. Both fixups defeat the purpose of having this
> test, because they don't use all 6 register arguments. We need to
> choose a different syscall; I believe splice() fits the bill. The other
nice, thanks for digging that up :) I've applied your suggested
changes (and added Suggested-by tag), thanks! Let's hope v2 will make
it to patchworks and we'll get BPF CI to test all this properly.
> alternatives that I rejected were:
>
> - clone() - argument order is messy;
> - recvfrom() - s390x uses socketcall instead;
> - ipc() - doesn't seem to be available on aarch64.
>
> The following worked for me:
>
> diff --git
> a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c
> b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c
> index e18dd82eb801..2900c5e9a016 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c
> @@ -1,20 +1,22 @@
> // SPDX-License-Identifier: GPL-2.0
> /* Copyright 2022 Sony Group Corporation */
> +#define _GNU_SOURCE
> +#include <fcntl.h>
> #include <sys/prctl.h>
> -#include <sys/mman.h>
> #include <test_progs.h>
> #include "bpf_syscall_macro.skel.h"
>
> void test_bpf_syscall_macro(void)
> {
> struct bpf_syscall_macro *skel = NULL;
> - int err, page_size = getpagesize();
> + int err;
> int exp_arg1 = 1001;
> unsigned long exp_arg2 = 12;
> unsigned long exp_arg3 = 13;
> unsigned long exp_arg4 = 14;
> unsigned long exp_arg5 = 15;
> - void *r;
> + loff_t off_in, off_out;
> + ssize_t r;
>
> /* check whether it can open program */
> skel = bpf_syscall_macro__open();
> @@ -71,18 +73,17 @@ void test_bpf_syscall_macro(void)
> ASSERT_EQ(skel->bss->arg4_syscall, exp_arg4,
> "BPF_KPROBE_SYSCALL_arg4");
> ASSERT_EQ(skel->bss->arg5_syscall, exp_arg5,
> "BPF_KPROBE_SYSCALL_arg5");
>
> - r = mmap((void *)0x12340000, 3 * page_size, PROT_READ |
> PROT_WRITE,
> - MAP_PRIVATE, -42, 5 * page_size);
> + r = splice(-42, &off_in, 42, &off_out, 0x12340000,
> SPLICE_F_NONBLOCK);
> err = -errno;
> - ASSERT_EQ(r, MAP_FAILED, "mmap_res");
> - ASSERT_EQ(err, -EBADF, "mmap_err");
> + ASSERT_EQ(r, -1, "splice_res");
> + ASSERT_EQ(err, -EBADF, "splice_err");
>
> - ASSERT_EQ(skel->bss->mmap_addr, 0x12340000, "mmap_arg1");
> - ASSERT_EQ(skel->bss->mmap_length, 3 * page_size, "mmap_arg2");
> - ASSERT_EQ(skel->bss->mmap_prot, PROT_READ | PROT_WRITE,
> "mmap_arg3");
> - ASSERT_EQ(skel->bss->mmap_flags, MAP_PRIVATE, "mmap_arg4");
> - ASSERT_EQ(skel->bss->mmap_fd, -42, "mmap_arg5");
> - ASSERT_EQ(skel->bss->mmap_offset, 5 * page_size, "mmap_arg6");
> + ASSERT_EQ(skel->bss->splice_fd_in, -42, "splice_arg1");
> + ASSERT_EQ(skel->bss->splice_off_in, (__u64)&off_in,
> "splice_arg2");
> + ASSERT_EQ(skel->bss->splice_fd_out, 42, "splice_arg3");
> + ASSERT_EQ(skel->bss->splice_off_out, (__u64)&off_out,
> "splice_arg4");
> + ASSERT_EQ(skel->bss->splice_len, 0x12340000, "splice_arg5");
> + ASSERT_EQ(skel->bss->splice_flags, SPLICE_F_NONBLOCK,
> "splice_arg6");
>
> cleanup:
> bpf_syscall_macro__destroy(skel);
> diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> index c07c5c52d5fc..1a476d8ed354 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> +++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> @@ -81,28 +81,28 @@ int BPF_KSYSCALL(prctl_enter, int option, unsigned
> long arg2,
> return 0;
> }
>
> -__u64 mmap_addr;
> -__u64 mmap_length;
> -__u64 mmap_prot;
> -__u64 mmap_flags;
> -__u64 mmap_fd;
> -__u64 mmap_offset;
> -
> -SEC("ksyscall/mmap")
> -int BPF_KSYSCALL(mmap_enter, void *addr, size_t length, int prot, int
> flags,
> - int fd, off_t offset)
> +__u64 splice_fd_in;
> +__u64 splice_off_in;
> +__u64 splice_fd_out;
> +__u64 splice_off_out;
> +__u64 splice_len;
> +__u64 splice_flags;
> +
> +SEC("ksyscall/splice")
> +int BPF_KSYSCALL(splice_enter, int fd_in, loff_t *off_in, int fd_out,
> + loff_t *off_out, size_t len, unsigned int flags)
> {
> pid_t pid = bpf_get_current_pid_tgid() >> 32;
>
> if (pid != filter_pid)
> return 0;
>
> - mmap_addr = (__u64)addr;
> - mmap_length = length;
> - mmap_prot = prot;
> - mmap_flags = flags;
> - mmap_fd = fd;
> - mmap_offset = offset;
> + splice_fd_in = fd_in;
> + splice_off_in = (__u64)off_in;
> + splice_fd_out = fd_out;
> + splice_off_out = (__u64)off_out;
> + splice_len = len;
> + splice_flags = flags;
>
> return 0;
> }
> --
> 2.39.0
>
> Best regards,
> Ilya
