On Tue, Nov 15, 2022 at 03:34:55PM -0800, Alexei Starovoitov wrote:
> On Tue, Nov 15, 2022 at 3:26 PM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
> >
> > On Tue, Nov 15, 2022 at 01:49:54PM +0100, Daniel Borkmann wrote:
> > > On 11/15/22 10:50 AM, Jiri Olsa wrote:
> > > > hi,
> > > > perf_event_bpf_event and bpf_audit_prog calls currently report zero
> > > > program id for unload path.
> > > >
> > > > It's because of the [1] change moved those audit calls into work queue
> > > > and they are executed after the id is zeroed in bpf_prog_free_id.
> > > >
> > > > I originally made a change that added 'id_audit' field to struct
> > > > bpf_prog, which would be initialized as id, untouched and used
> > > > in audit callbacks.
> > > >
> > > > Then I realized we might actually not need to zero prog->aux->id
> > > > in bpf_prog_free_id. It seems to be called just once on release
> > > > paths. Tests seems ok with that.
> > > >
> > > > thoughts?
> > > >
> > > > thanks,
> > > > jirka
> > > >
> > > >
> > > > [1] d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq context.")
> > > > ---
> > > > kernel/bpf/syscall.c | 1 -
> > > > 1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > > index fdbae52f463f..426529355c29 100644
> > > > --- a/kernel/bpf/syscall.c
> > > > +++ b/kernel/bpf/syscall.c
> > > > @@ -1991,7 +1991,6 @@ void bpf_prog_free_id(struct bpf_prog *prog, bool do_idr_lock)
> > > > __acquire(&prog_idr_lock);
> > > > idr_remove(&prog_idr, prog->aux->id);
> > > > - prog->aux->id = 0;
> > >
> > > This would trigger a race when offloaded progs are used, see also ad8ad79f4f60 ("bpf:
> > > offload: free program id when device disappears"). __bpf_prog_offload_destroy() calls
> > > it, and my read is that later bpf_prog_free_id() then hits the early !prog->aux->id
> > > return path. Is there a reason for irq context to not defer the bpf_prog_free_id()?
> >
> > there's comment saying:
> > /* bpf_prog_free_id() must be called first */
> >
> > it was added together with the BPF_(PROG|MAP)_GET_NEXT_ID api:
> > 34ad5580f8f9 bpf: Add BPF_(PROG|MAP)_GET_NEXT_ID command
> >
> > Martin, any idea?
> >
> > while looking on this I noticed we can remove the do_idr_lock argument
> > (patch below) because it's always true and I think we need to upgrade
> > all the prog_idr_lock locks to spin_lock_irqsave because bpf_prog_put
> > could be called from irq context because of d809e134be7a
>
> before we dive into rabbit hole let's understand
> the severity of
> "perf_event_bpf_event and bpf_audit_prog calls currently report zero
> program id for unload path"
>
> and why we cannot leave it as-is.
I found this because I wanted use perf bpf-event to monitor bpf
programs loads/unloads.. and I need 'id' on the unload event
perf_event_bpf_event is irq safe so quick fix for me would be
to move it back:
if (atomic64_dec_and_test(&aux->refcnt)) {
+ perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_UNLOAD, 0);
/* bpf_prog_free_id() must be called first */
bpf_prog_free_id(prog, do_idr_lock);
FWIW I also used fix below for a while for testing
jirka
---
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d88e0741b381..6b752d4815e6 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1163,6 +1163,7 @@ struct bpf_prog_aux {
u32 max_tp_access;
u32 stack_depth;
u32 id;
+ u32 id_audit;
u32 func_cnt; /* used by non-func prog as the number of func progs */
u32 func_idx; /* 0 for non-func prog, the index in func array for func prog */
u32 attach_btf_id; /* in-kernel BTF type id to attach to */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index d6081e8336c6..1f4fdf0aaac6 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1930,7 +1930,7 @@ static void bpf_audit_prog(const struct bpf_prog *prog, unsigned int op)
if (unlikely(!ab))
return;
audit_log_format(ab, "prog-id=%u op=%s",
- prog->aux->id, bpf_audit_str[op]);
+ prog->aux->id_audit, bpf_audit_str[op]);
audit_log_end(ab);
}
@@ -1942,7 +1942,7 @@ static int bpf_prog_alloc_id(struct bpf_prog *prog)
spin_lock_bh(&prog_idr_lock);
id = idr_alloc_cyclic(&prog_idr, prog, 1, INT_MAX, GFP_ATOMIC);
if (id > 0)
- prog->aux->id = id;
+ prog->aux->id = prog->aux->id_audit = id;
spin_unlock_bh(&prog_idr_lock);
idr_preload_end();
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 0f0bfcf5adce..32edb3a4360e 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -9065,7 +9065,7 @@ void perf_event_bpf_event(struct bpf_prog *prog,
},
.type = type,
.flags = flags,
- .id = prog->aux->id,
+ .id = prog->aux->id_audit,
},
};
