| Message ID | 20230106154400.74211-2-paul@paul-moore.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e7895f017b79410bf4591396a733b876dc1e0e9d |
| Delegated to: | BPF |
| Headers | show |
| Series | [v3,1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD | expand |
On Fri, Jan 6, 2023 at 7:44 AM Paul Moore <paul@paul-moore.com> wrote: > > It was determined that the do_idr_lock parameter to > bpf_prog_free_id() was not necessary as it should always be true. > > Suggested-by: Stanislav Fomichev <sdf@google.com> nit: I believe it's been suggested several times by different people Acked-by: Stanislav Fomichev <sdf@google.com> > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > * v3 > - initial draft > --- > include/linux/bpf.h | 2 +- > kernel/bpf/syscall.c | 20 ++++++-------------- > 2 files changed, 7 insertions(+), 15 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 3de24cfb7a3d..634d37a599fa 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1832,7 +1832,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/syscall.c b/kernel/bpf/syscall.c > index 61bb19e81b9c..ecca9366c7a6 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -2001,7 +2001,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; > > @@ -2013,18 +2013,10 @@ 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) > @@ -2067,11 +2059,11 @@ 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_free_id(prog); > __bpf_prog_put_noref(prog, true); > } > > -static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock) > +static void __bpf_prog_put(struct bpf_prog *prog) > { > struct bpf_prog_aux *aux = prog->aux; > > @@ -2087,7 +2079,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock) > > void bpf_prog_put(struct bpf_prog *prog) > { > - __bpf_prog_put(prog, true); > + __bpf_prog_put(prog); > } > EXPORT_SYMBOL_GPL(bpf_prog_put); > > -- > 2.39.0 >
On Fri, Jan 6, 2023 at 2:45 PM Stanislav Fomichev <sdf@google.com> wrote: > > On Fri, Jan 6, 2023 at 7:44 AM Paul Moore <paul@paul-moore.com> wrote: > > > > It was determined that the do_idr_lock parameter to > > bpf_prog_free_id() was not necessary as it should always be true. > > > > Suggested-by: Stanislav Fomichev <sdf@google.com> > > nit: I believe it's been suggested several times by different people As much as I would like to follow all of the kernel relevant mailing lists, I'm short about 30hrs in a day to do that, and you were the first one I saw suggesting that change :) > Acked-by: Stanislav Fomichev <sdf@google.com>
On 01/09, Paul Moore wrote: > On Fri, Jan 6, 2023 at 2:45 PM Stanislav Fomichev <sdf@google.com> wrote: > > > > On Fri, Jan 6, 2023 at 7:44 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > It was determined that the do_idr_lock parameter to > > > bpf_prog_free_id() was not necessary as it should always be true. > > > > > > Suggested-by: Stanislav Fomichev <sdf@google.com> > > > > nit: I believe it's been suggested several times by different people > As much as I would like to follow all of the kernel relevant mailing > lists, I'm short about 30hrs in a day to do that, and you were the > first one I saw suggesting that change :) Sure, sure, I'm just stating it for the other people on the CC. So maybe we can drop this line when applying. > > Acked-by: Stanislav Fomichev <sdf@google.com> > -- > paul-moore.com
On Mon, Jan 9, 2023 at 12:58 PM <sdf@google.com> wrote: > On 01/09, Paul Moore wrote: > > On Fri, Jan 6, 2023 at 2:45 PM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > On Fri, Jan 6, 2023 at 7:44 AM Paul Moore <paul@paul-moore.com> wrote: > > > > > > > > It was determined that the do_idr_lock parameter to > > > > bpf_prog_free_id() was not necessary as it should always be true. > > > > > > > > Suggested-by: Stanislav Fomichev <sdf@google.com> > > > > > > nit: I believe it's been suggested several times by different people > > > As much as I would like to follow all of the kernel relevant mailing > > lists, I'm short about 30hrs in a day to do that, and you were the > > first one I saw suggesting that change :) > > Sure, sure, I'm just stating it for the other people on the CC. So maybe > we can drop this line when applying. That's fine with me. To be honest, you folks can do whatever tweaks you want to these patches and that's okay with me; or even just dump them and rewrite them as you see fit, if that's easier. I'm only concerned with getting the regression fixed, who fixes it isn't something I'm worried about.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 3de24cfb7a3d..634d37a599fa 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1832,7 +1832,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/syscall.c b/kernel/bpf/syscall.c index 61bb19e81b9c..ecca9366c7a6 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2001,7 +2001,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; @@ -2013,18 +2013,10 @@ 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) @@ -2067,11 +2059,11 @@ 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_free_id(prog); __bpf_prog_put_noref(prog, true); } -static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock) +static void __bpf_prog_put(struct bpf_prog *prog) { struct bpf_prog_aux *aux = prog->aux; @@ -2087,7 +2079,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock) void bpf_prog_put(struct bpf_prog *prog) { - __bpf_prog_put(prog, true); + __bpf_prog_put(prog); } EXPORT_SYMBOL_GPL(bpf_prog_put);
It was determined that the do_idr_lock parameter to bpf_prog_free_id() was not necessary as it should always be true. Suggested-by: Stanislav Fomichev <sdf@google.com> Signed-off-by: Paul Moore <paul@paul-moore.com> --- * v3 - initial draft --- include/linux/bpf.h | 2 +- kernel/bpf/syscall.c | 20 ++++++-------------- 2 files changed, 7 insertions(+), 15 deletions(-)