Re: [RFC bpf-next] bpf: Fix perf bpf event and audit prog id logging

[Jiri Olsa <olsajiri@xxxxxxxxx>]

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

thanks,
jirka


---
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 49f9d2bec401..50c71ce698d0 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1761,7 +1761,7 @@ void bpf_prog_inc(struct bpf_prog *prog);
 struct bpf_prog * __must_check bpf_prog_inc_not_zero(struct bpf_prog *prog);
 void bpf_prog_put(struct bpf_prog *prog);
 
-void bpf_prog_free_id(struct bpf_prog *prog, bool do_idr_lock);
+void bpf_prog_free_id(struct bpf_prog *prog);
 void bpf_map_free_id(struct bpf_map *map, bool do_idr_lock);
 
 struct btf_field *btf_record_find(const struct btf_record *rec,
diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
index 13e4efc971e6..327dab644200 100644
--- a/kernel/bpf/offload.c
+++ b/kernel/bpf/offload.c
@@ -217,7 +217,7 @@ static void __bpf_prog_offload_destroy(struct bpf_prog *prog)
 		offload->offdev->ops->destroy(prog);
 
 	/* Make sure BPF_PROG_GET_NEXT_ID can't find this dead program */
-	bpf_prog_free_id(prog, true);
+	bpf_prog_free_id(prog);
 
 	list_del_init(&offload->offloads);
 	kfree(offload);
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index fdbae52f463f..9b929d8ab06d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1973,7 +1973,7 @@ static int bpf_prog_alloc_id(struct bpf_prog *prog)
 	return id > 0 ? 0 : id;
 }
 
-void bpf_prog_free_id(struct bpf_prog *prog, bool do_idr_lock)
+void bpf_prog_free_id(struct bpf_prog *prog)
 {
 	unsigned long flags;
 
@@ -1985,18 +1985,12 @@ void bpf_prog_free_id(struct bpf_prog *prog, bool do_idr_lock)
 	if (!prog->aux->id)
 		return;
 
-	if (do_idr_lock)
-		spin_lock_irqsave(&prog_idr_lock, flags);
-	else
-		__acquire(&prog_idr_lock);
+	spin_lock_irqsave(&prog_idr_lock, flags);
 
 	idr_remove(&prog_idr, prog->aux->id);
 	prog->aux->id = 0;
 
-	if (do_idr_lock)
-		spin_unlock_irqrestore(&prog_idr_lock, flags);
-	else
-		__release(&prog_idr_lock);
+	spin_unlock_irqrestore(&prog_idr_lock, flags);
 }
 
 static void __bpf_prog_put_rcu(struct rcu_head *rcu)
@@ -2048,7 +2042,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock)
 
 	if (atomic64_dec_and_test(&aux->refcnt)) {
 		/* bpf_prog_free_id() must be called first */
-		bpf_prog_free_id(prog, do_idr_lock);
+		bpf_prog_free_id(prog);
 
 		if (in_irq() || irqs_disabled()) {
 			INIT_WORK(&aux->work, bpf_prog_put_deferred);