On Fri, Jan 6, 2023 at 7:44 AM Paul Moore <p...@paul-moore.com> wrote: > > When changing the ebpf program put() routines to support being called > from within IRQ context the program ID was reset to zero prior to > calling the perf event and audit UNLOAD record generators, which > resulted in problems as the ebpf program ID was bogus (always zero). > This patch addresses this problem by removing an unnecessary call to > bpf_prog_free_id() in __bpf_prog_offload_destroy() and adjusting > __bpf_prog_put() to only call bpf_prog_free_id() after audit and perf > have finished their bpf program unload tasks in > bpf_prog_put_deferred(). For the record, no one can determine, or > remember, why it was necessary to free the program ID, and remove it > from the IDR, prior to executing bpf_prog_put_deferred(); > regardless, both Stanislav and Alexei agree that the approach in this > patch should be safe. > > It is worth noting that when moving the bpf_prog_free_id() call, the > do_idr_lock parameter was forced to true as the ebpf devs determined > this was the correct as the do_idr_lock should always be true. The > do_idr_lock parameter will be removed in a follow-up patch, but it > was kept here to keep the patch small in an effort to ease any stable > backports. > > I also modified the bpf_audit_prog() logic used to associate the > AUDIT_BPF record with other associated records, e.g. @ctx != NULL. > Instead of keying off the operation, it now keys off the execution > context, e.g. '!in_irg && !irqs_disabled()', which is much more > appropriate and should help better connect the UNLOAD operations with > the associated audit state (other audit records). > > Cc: sta...@vger.kernel.org > Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq > context.") > Reported-by: Burn Alting <burn.alt...@iinet.net.au> > Reported-by: Jiri Olsa <olsaj...@gmail.com> > Suggested-by: Stanislav Fomichev <s...@google.com> > Suggested-by: Alexei Starovoitov <alexei.starovoi...@gmail.com> > Signed-off-by: Paul Moore <p...@paul-moore.com>
Acked-by: Stanislav Fomichev <s...@google.com> Thank you! There might be a chance it breaks test_offload.py (I don't remember whether it checks this prog-is-removed-from-id part or not), but I don't think it's fair to ask to address it :-) Since it doesn't trigger in CI, I'll take another look next week when doing a respin of my 'xdp-hints' series. > --- > * v3 > - abandon most of the changes in v2 > - move bpf_prog_free_id() after the audit/perf unload hooks > - remove bpf_prog_free_id() from __bpf_prog_offload_destroy() > - added stable tag > * v2 > - change subj > - add mention of the perf regression > - drop the dedicated program audit ID > - add the bpf_prog::valid_id flag, bpf_prog_get_id() getter > - convert prog ID users to new ID getter > * v1 > - subj was: "bpf: restore the ebpf audit UNLOAD id field" > - initial draft > --- > kernel/bpf/offload.c | 3 --- > kernel/bpf/syscall.c | 6 ++---- > 2 files changed, 2 insertions(+), 7 deletions(-) > > diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c > index 13e4efc971e6..190d9f9dc987 100644 > --- a/kernel/bpf/offload.c > +++ b/kernel/bpf/offload.c > @@ -216,9 +216,6 @@ static void __bpf_prog_offload_destroy(struct bpf_prog > *prog) > if (offload->dev_state) > offload->offdev->ops->destroy(prog); > > - /* Make sure BPF_PROG_GET_NEXT_ID can't find this dead program */ > - bpf_prog_free_id(prog, true); > - > list_del_init(&offload->offloads); > kfree(offload); > prog->aux->offload = NULL; > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 64131f88c553..61bb19e81b9c 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -1972,7 +1972,7 @@ static void bpf_audit_prog(const struct bpf_prog *prog, > unsigned int op) > return; > if (audit_enabled == AUDIT_OFF) > return; > - if (op == BPF_AUDIT_LOAD) > + if (!in_irq() && !irqs_disabled()) > ctx = audit_context(); > ab = audit_log_start(ctx, GFP_ATOMIC, AUDIT_BPF); > if (unlikely(!ab)) > @@ -2067,6 +2067,7 @@ static void bpf_prog_put_deferred(struct work_struct > *work) > prog = aux->prog; > perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_UNLOAD, 0); > bpf_audit_prog(prog, BPF_AUDIT_UNLOAD); > + bpf_prog_free_id(prog, true); > __bpf_prog_put_noref(prog, true); > } > > @@ -2075,9 +2076,6 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool > do_idr_lock) > struct bpf_prog_aux *aux = prog->aux; > > if (atomic64_dec_and_test(&aux->refcnt)) { > - /* bpf_prog_free_id() must be called first */ > - bpf_prog_free_id(prog, do_idr_lock); > - > if (in_irq() || irqs_disabled()) { > INIT_WORK(&aux->work, bpf_prog_put_deferred); > schedule_work(&aux->work); > -- > 2.39.0 > -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit