On Tue, Jun 07, 2022 at 09:29:30PM -0700, Alexei Starovoitov wrote:
> 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.
how about using bpf_prog_active just to disable instrumentation,
and only check it before running bpf prog
plus adding prog->active check for both kprobe and kprobe_multi
to bpf_prog_run function, like in the patch below
jirka
---
diff --git a/include/linux/filter.h b/include/linux/filter.h
index ed0c0ff42ad5..a27e947f8749 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -632,7 +632,12 @@ static __always_inline u32 __bpf_prog_run(const struct bpf_prog *prog,
static __always_inline u32 bpf_prog_run(const struct bpf_prog *prog, const void *ctx)
{
- return __bpf_prog_run(prog, ctx, bpf_dispatcher_nop_func);
+ u32 ret = 0;
+
+ if (likely(__this_cpu_inc_return(*(prog->active)) == 1))
+ ret = __bpf_prog_run(prog, ctx, bpf_dispatcher_nop_func);
+ __this_cpu_dec(*(prog->active));
+ return ret;
}
/*
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 10b157a6d73e..62389ff0f15a 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -103,16 +103,8 @@ unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx)
cant_sleep();
- if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
- /*
- * since some bpf program is already running on this cpu,
- * don't call into another bpf program (same or different)
- * and don't send kprobe event into ring-buffer,
- * so return zero here
- */
- ret = 0;
- goto out;
- }
+ if (unlikely(this_cpu_read(bpf_prog_active)))
+ return 0;
/*
* Instead of moving rcu_read_lock/rcu_dereference/rcu_read_unlock
@@ -133,10 +125,6 @@ unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx)
ret = bpf_prog_run_array(rcu_dereference(call->prog_array),
ctx, bpf_prog_run);
rcu_read_unlock();
-
- out:
- __this_cpu_dec(bpf_prog_active);
-
return ret;
}
@@ -2395,10 +2383,8 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
struct bpf_run_ctx *old_run_ctx;
int err;
- if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
- err = 0;
- goto out;
- }
+ if (unlikely(this_cpu_read(bpf_prog_active)))
+ return 0;
migrate_disable();
rcu_read_lock();
@@ -2407,9 +2393,6 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
bpf_reset_run_ctx(old_run_ctx);
rcu_read_unlock();
migrate_enable();
-
- out:
- __this_cpu_dec(bpf_prog_active);
return err;
}
