On Wed, Jul 13, 2022 at 11:57 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> On Wed, Jul 13, 2022 at 10:57 AM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Wed, Jul 13, 2022 at 9:29 AM <sdf@xxxxxxxxxx> wrote:
> > >
> > > On 07/12, Andrii Nakryiko wrote:
> > > > Convert few selftest that used plain SEC("kprobe") with arch-specific
> > > > syscall wrapper prefix to ksyscall/kretsyscall and corresponding
> > > > BPF_KSYSCALL macro. test_probe_user.c is especially benefiting from this
> > > > simplification.
> > >
> > > That looks super nice! I'm assuming the goal is probably
> >
> > Thanks!
> >
> > > to get rid of that SYS_PREFIX everywhere eventually? And have a simple
> > > test that exercises fentry/etc parsing?
> >
> > All the other uses of SYS_PREFIX in selftests right now are
> > fentry/fexit. If the consensus is that this sort of higher-level
> > wrapper around fentry/fexit specifically for syscalls is useful, it's
> > not a lot of work to add something like SEC("fsyscall") and
> > SEC("fretsyscall") with the same approach.
> >
> > One possible argument against this (and I need to double check my
> > assumptions first), is that with SYSCALL_WRAPPER used (which is true
> > for "major" platforms like x86_64), fentry doesn't provide much
> > benefit because __<arch>_sys_<syscall>() function will have only one
> > typed argument - struct pt_regs, and so we'll have to use
> > BPF_CORE_READ() to fetch actual arguments, at which point BPF verifier
> > will lose track of type information. So it's just a slightly more
> > performant (in terms of invocation overhead) kprobe at that point, but
> > with no added benefit of BTF types for input arguments.
> >
> > But curious to hear what others think about this.
>
> What would be nice (but not sure if possible, I haven't looked
> closely), if these same ksyscall sections would pick the best
> underlying implementation: if fentry is available -> attach to fentry,
> if not -> fallback to kprobe (and do all this __<prefix>_sys vs __sys
> dance behind the scenes). Any reasons the users should care if it's
> really a kprobe or an fentry?
It's technically possible to choose kprobe vs fentry, but I'm not
comfortable with that level of autonomy for libbpf. There might be
subtle differences between kprobes and fentry (and I did run into some
limitations with fentry due to extra BTF information that verifier
enforces, while I need to only get raw integer value of some pointer;
had to work around that in retsnoop, for example), so I generally
follow the philosophy that user needs to be explicit about what they
want, and libbpf shouldn't try to guess (which differs from BCC's
approach in a number of areas and I'm pretty pleased how that turns
out for libbpf and its users in general).
So if we do this shim for fentry/fexit, I think it should be explicit
SEC("fsyscall/fretsyscall") or something along those lines.
>
>
>
> > > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > > > ---
> > > > .../selftests/bpf/progs/bpf_syscall_macro.c | 6 ++---
> > > > .../selftests/bpf/progs/test_attach_probe.c | 15 +++++------
> > > > .../selftests/bpf/progs/test_probe_user.c | 27 +++++--------------
> > > > 3 files changed, 16 insertions(+), 32 deletions(-)
> > >
> >
> > [...]
