On Tue, May 31, 2022 at 11:07:31AM -0700, Alexei Starovoitov wrote: > On Sun, May 29, 2022 at 3:06 PM Daniel Xu <dxu@xxxxxxxxx> wrote: > > > > This commit adds PROG_TEST_RUN support to BPF_PROG_TYPE_KPROBE progs. On > > top of being generally useful for unit testing kprobe progs, this commit > > more specifically helps solve a relability problem with bpftrace BEGIN > > and END probes. > > > > BEGIN and END probes are run exactly once at the beginning and end of a > > bpftrace tracing session, respectively. bpftrace currently implements > > the probes by creating two dummy functions and attaching the BEGIN and > > END progs, if defined, to those functions and calling the dummy > > functions as appropriate. This works pretty well most of the time except > > for when distros strip symbols from bpftrace. Every now and then this > > happens and users get confused. Having PROG_TEST_RUN support will help > > solve this issue by allowing us to directly trigger uprobes from > > userspace. > > > > Admittedly, this is a pretty specific problem and could probably be > > solved other ways. However, PROG_TEST_RUN also makes unit testing more > > convenient, especially as users start building more complex tracing > > applications. So I see this as killing two birds with one stone. > > bpftrace approach of uprobe-ing into BEGIN_trigger can > be solved with raw_tp prog. > "BEGIN" bpftrace's program doesn't have to be uprobe. > I can be raw_tp and prog_test_run command is > already implemented for raw_tp. > > kprobe prog has pt_regs as arguments, > raw_tp has tracepoint args. > Both progs expect kernel addresses in args. > Passing 'struct pt_regs' filled with integers and non-kernel addresses > won't help to unit test bpf-kprobe programs. > There is little use in creating a dummy kprobe-bpf prog > that expects RDI to be 1 and RSI to be 2 > (like selftest from patch 2 does) and running it. Yeah that's a good point about the kprobe case. But AFAICT uprobes are implemented using a kprobe prog in which case it would be both possible and useful to insert userspace args and userspace addrspace addrs. > We already have raw_tp with similar args and such > progs can be executed already. > Whether SEC() part says kprobe/ or raw_tp/ doesn't change > much in the prog itself. I suppose so, and I guess you could always bpf_program__set_type(..). > More so raw_tp prog will work on all architectures, > whereas kprobe's pt_regs are arch specific. > So even if kprobe progs were runnable, bpftrace > should probably be using raw_tp to be arch independent. bpftrace has all the infra to pull arbitrary structs out of vmlinux BTF already. It should be fairly simple to get the arch-specific struct pt_regs size and construct a buffer of all 0s. And fall back to old logic (that'll be necessary for a while) if kprobe BPF_PROG_RUN or vmlinux BTF is missing. That being said, I didn't realize that when I put up the patch, so thanks for the hint. It sounds like it's probably simpler to just use raw_tp then. FWIW I still think this feature is useful, but since I probably won't use it in bpftrace I'm fine with dropping the patchset. If anyone still wants it in I'm also fine with continuing on it. Thanks, Daniel
