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.
