On Tue, May 31, 2022 at 4:24 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Wed, May 25, 2022 at 4:40 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote: > > > > hi, > > Alexei suggested to use prog->active instead global bpf_prog_active > > for programs attached with kprobe multi [1]. > > > > AFAICS this will bypass bpf_disable_instrumentation, which seems to be > > ok for some places like hash map update, but I'm not sure about other > > places, hence this is RFC post. > > > > I'm not sure how are kprobes different to trampolines in this regard, > > because trampolines use prog->active and it's not a problem. > > > > thoughts? > > > > Let's say we have two kernel functions A and B? B can be called from > BPF program though some BPF helper, ok? Now let's say I have two BPF > programs kprobeX and kretprobeX, both are attached to A and B. With > using prog->active instead of per-cpu bpf_prog_active, what would be > the behavior when A is called somewhere in the kernel. > > 1. A is called > 2. kprobeX is activated for A, calls some helper which eventually calls B > 3. kprobeX is attempted to be called for B, but is skipped due to prog->active > 4. B runs > 5. kretprobeX is activated for B, calls some helper which eventually calls B > 6. kprobeX is ignored (prog->active > 0) > 7. B runs > 8. kretprobeX is ignored (prog->active > 0) > 9. kretprobeX is activated for A, calls helper which calls B > 10. kprobeX is activated for B > 11. kprobeX is ignored (prog->active > 0) not correct. kprobeX actually runs. but the end result is correct. > 12. B runs > 13. kretprobeX is ignored (prog->active > 0) > 14. B runs > 15. kretprobeX is ignored (prog->active > 0) > > > If that's correct, we get: > > 1. kprobeX for A > 2. kretprobeX for B > 3. kretprobeX for A > 4. kprobeX for B Here it's correct. > It's quite mind-boggling and annoying in practice. I'd very much > prefer just kprobeX for A followed by kretprobeX for A. That's it. > > I'm trying to protect against this in retsnoop with custom per-cpu > logic in each program, but I so much more prefer bpf_prog_active, > which basically says "no nested kprobe calls while kprobe program is > running", which makes a lot of sense in practice. It makes sense for retsnoop, but does not make sense in general. > Given kprobe already used global bpf_prog_active I'd say multi-kprobe > should stick to bpf_prog_active as well. I strongly disagree. Both multi kprobe and kprobe should move to per prog counter plus some other protection (we cannot just move to per-prog due to syscalls). It's true that the above order is mind-boggling, but it's much better than missing kprobe invocation completely just because another kprobe is running on the same cpu. People complained numerous times about this kprobe behavior. kprobeX attached to A kprobeY attached to B. If kprobeX calls B kprobeY is not going to be called. Means that anything that bpf is using is lost. spin locks, lists, rcu, etc. Sleepable uprobes are coming. iirc Delyan's patch correctly. We will do migrate_disable and inc bpf_prog_active. Now random kprobes on that cpu will be lost. It's awful. We have to fix it.
