On Wed, Jul 13, 2022 at 1:30 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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.
In this case yeah, if there are user-visible differences, doing a
separate fsyscall is better.
(removing SYS_PREFIX seems like a good goal)
