On Wed, Dec 1, 2021 at 9:37 AM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Tue, Nov 30, 2021 at 11:13 PM Andrii Nakryiko > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > Hm... I'd actually try to keep kprobe BTF-free. We have fentry for > > cases where BTF is present and the function is simple enough (like <=6 > > args, etc). Kprobe is an escape hatch mechanism when all the BTF > > fanciness just gets in the way (retsnoop being a primary example from > > my side). What I meant here was that bpf_get_arg(int n) would read > > correct fields from pt_regs that map to first N arguments passed in > > the registers. What we currently have with PT_REGS_PARM macros in > > bpf_tracing.h, but with a proper unified BPF helper. > > and these macros are arch specific. > which means that it won't be a trivial patch to add bpf_get_arg() > support for kprobes. no one suggested it would be trivial :) things worth doing are usually non-trivial, as can be evidenced by Jiri's patch set > Plenty of things to consider. Like should it return an error > at run-time or verification time when a particular arch is not supported. See my other replies to Jiri, I'm more and more convinced that dynamic is the way to go for things like this, where the safety of the kernel or BPF program are not compromised. But you emphasized an important point, that it's probably good to allow users to distinguish errors from reading actual value 0. There are and will be situations where argument isn't available or some combination of conditions are not supported. So I think, while it's a bit more verbose, these forms are generally better: int bpf_get_func_arg(int n, u64 *value); int bpf_get_func_ret(u64 *value); WDYT? > Or argument 6 might be available on one arch, but not on the other. > 32-bit CPU regs vs 64-bit regs of BPF, etc. > I wouldn't attempt to mix this work with current patches. Oh, I didn't suggest doing it as part of this already huge and complicated set. But I think it's good to think a bit ahead and design the helper API appropriately, at the very least. And again, I think bpf_get_func_arg/bpf_get_func_ret deserve their own patch set where we can discuss all this independently from multi-attach.
