Re: [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD

On Fri, Jan 6, 2023 at 7:44 AM Paul Moore <paul(a)paul-moore.com&gt; 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: stable(a)vger.kernel.org > Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq context.") > Reported-by: Burn Alting <burn.alting(a)iinet.net.au&gt; > Reported-by: Jiri Olsa <olsajiri(a)gmail.com&gt; > Suggested-by: Stanislav Fomichev <sdf(a)google.com&gt; > Suggested-by: Alexei Starovoitov <alexei.starovoitov(a)gmail.com&gt; > Signed-off-by: Paul Moore <paul(a)paul-moore.com&gt; Acked-by: Stanislav Fomichev <sdf(a)google.com&gt; 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.