* [PATCH v3 2/2] bpf: remove the do_idr_lock parameter from bpf_prog_free_id()
2023-01-06 15:43 [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD Paul Moore
@ 2023-01-06 15:44 ` Paul Moore
2023-01-06 19:45 ` Stanislav Fomichev
2023-01-06 19:45 ` [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD Stanislav Fomichev
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Paul Moore @ 2023-01-06 15:44 UTC (permalink / raw)
To: linux-audit, bpf; +Cc: Burn Alting, Stanislav Fomichev, Alexei Starovoitov
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(-)
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] bpf: remove the do_idr_lock parameter from bpf_prog_free_id()
2023-01-06 15:44 ` [PATCH v3 2/2] bpf: remove the do_idr_lock parameter from bpf_prog_free_id() Paul Moore
@ 2023-01-06 19:45 ` Stanislav Fomichev
2023-01-09 16:57 ` Paul Moore
0 siblings, 1 reply; 12+ messages in thread
From: Stanislav Fomichev @ 2023-01-06 19:45 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, bpf, Burn Alting, Alexei Starovoitov
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] bpf: remove the do_idr_lock parameter from bpf_prog_free_id()
2023-01-06 19:45 ` Stanislav Fomichev
@ 2023-01-09 16:57 ` Paul Moore
2023-01-09 17:58 ` sdf
0 siblings, 1 reply; 12+ messages in thread
From: Paul Moore @ 2023-01-09 16:57 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: linux-audit, bpf, Burn Alting, Alexei Starovoitov
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>
--
paul-moore.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] bpf: remove the do_idr_lock parameter from bpf_prog_free_id()
2023-01-09 16:57 ` Paul Moore
@ 2023-01-09 17:58 ` sdf
2023-01-09 18:02 ` Paul Moore
0 siblings, 1 reply; 12+ messages in thread
From: sdf @ 2023-01-09 17:58 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, bpf, Burn Alting, Alexei Starovoitov
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] bpf: remove the do_idr_lock parameter from bpf_prog_free_id()
2023-01-09 17:58 ` sdf
@ 2023-01-09 18:02 ` Paul Moore
0 siblings, 0 replies; 12+ messages in thread
From: Paul Moore @ 2023-01-09 18:02 UTC (permalink / raw)
To: sdf; +Cc: linux-audit, bpf, Burn Alting, Alexei Starovoitov
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.
--
paul-moore.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD
2023-01-06 15:43 [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD Paul Moore
2023-01-06 15:44 ` [PATCH v3 2/2] bpf: remove the do_idr_lock parameter from bpf_prog_free_id() Paul Moore
@ 2023-01-06 19:45 ` Stanislav Fomichev
2023-01-09 16:54 ` Paul Moore
2023-01-10 4:00 ` patchwork-bot+netdevbpf
2023-01-10 9:10 ` Jiri Olsa
3 siblings, 1 reply; 12+ messages in thread
From: Stanislav Fomichev @ 2023-01-06 19:45 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, bpf, Burn Alting, Alexei Starovoitov
On Fri, Jan 6, 2023 at 7:44 AM Paul Moore <paul@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: stable@vger.kernel.org
> Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq context.")
> Reported-by: Burn Alting <burn.alting@iinet.net.au>
> Reported-by: Jiri Olsa <olsajiri@gmail.com>
> Suggested-by: Stanislav Fomichev <sdf@google.com>
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
Acked-by: Stanislav Fomichev <sdf@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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD
2023-01-06 19:45 ` [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD Stanislav Fomichev
@ 2023-01-09 16:54 ` Paul Moore
2023-01-09 18:04 ` sdf
0 siblings, 1 reply; 12+ messages in thread
From: Paul Moore @ 2023-01-09 16:54 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: linux-audit, bpf, Burn Alting, Alexei Starovoitov
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:
> >
> > 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@vger.kernel.org
> > Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq context.")
> > Reported-by: Burn Alting <burn.alting@iinet.net.au>
> > Reported-by: Jiri Olsa <olsajiri@gmail.com>
> > Suggested-by: Stanislav Fomichev <sdf@google.com>
> > Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
>
> Acked-by: Stanislav Fomichev <sdf@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.
No problem, I'm glad we found a solution that works for everyone; and
thank you for chasing down any test changes that may be necessary.
I'd like to get this patch into Linus' tree sooner rather than later
as it fixes a kinda ugly problem, would you be okay if this went in
via the bpf tree? With the appropriate ACKs I could send it to Linus
via the audit tree, but I think it would be much better to send it via
the bpf/netdev tree.
--
paul-moore.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD
2023-01-09 16:54 ` Paul Moore
@ 2023-01-09 18:04 ` sdf
0 siblings, 0 replies; 12+ messages in thread
From: sdf @ 2023-01-09 18:04 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, bpf, Burn Alting, Alexei Starovoitov
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:
> > >
> > > 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@vger.kernel.org
> > > Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from
> irq context.")
> > > Reported-by: Burn Alting <burn.alting@iinet.net.au>
> > > Reported-by: Jiri Olsa <olsajiri@gmail.com>
> > > Suggested-by: Stanislav Fomichev <sdf@google.com>
> > > Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > > Signed-off-by: Paul Moore <paul@paul-moore.com>
> >
> > Acked-by: Stanislav Fomichev <sdf@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.
> No problem, I'm glad we found a solution that works for everyone; and
> thank you for chasing down any test changes that may be necessary.
> I'd like to get this patch into Linus' tree sooner rather than later
> as it fixes a kinda ugly problem, would you be okay if this went in
> via the bpf tree? With the appropriate ACKs I could send it to Linus
> via the audit tree, but I think it would be much better to send it via
> the bpf/netdev tree.
Don't see any reason that this should go via bpf-next, so assuming
going via bpf three should be fine.
> --
> paul-moore.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD
2023-01-06 15:43 [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD Paul Moore
2023-01-06 15:44 ` [PATCH v3 2/2] bpf: remove the do_idr_lock parameter from bpf_prog_free_id() Paul Moore
2023-01-06 19:45 ` [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD Stanislav Fomichev
@ 2023-01-10 4:00 ` patchwork-bot+netdevbpf
2023-01-10 9:10 ` Jiri Olsa
3 siblings, 0 replies; 12+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-01-10 4:00 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, bpf, burn.alting, sdf, ast
Hello:
This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Fri, 6 Jan 2023 10:43:59 -0500 you 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.
>
> [...]
Here is the summary with links:
- [v3,1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD
https://git.kernel.org/bpf/bpf/c/ef01f4e25c17
- [v3,2/2] bpf: remove the do_idr_lock parameter from bpf_prog_free_id()
https://git.kernel.org/bpf/bpf/c/e7895f017b79
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD
2023-01-06 15:43 [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD Paul Moore
` (2 preceding siblings ...)
2023-01-10 4:00 ` patchwork-bot+netdevbpf
@ 2023-01-10 9:10 ` Jiri Olsa
2023-01-10 16:55 ` Paul Moore
3 siblings, 1 reply; 12+ messages in thread
From: Jiri Olsa @ 2023-01-10 9:10 UTC (permalink / raw)
To: Paul Moore
Cc: linux-audit, bpf, Burn Alting, Stanislav Fomichev, Alexei Starovoitov
On Fri, Jan 06, 2023 at 10:43:59AM -0500, Paul Moore 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@vger.kernel.org
> Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq context.")
> Reported-by: Burn Alting <burn.alting@iinet.net.au>
> Reported-by: Jiri Olsa <olsajiri@gmail.com>
> Suggested-by: Stanislav Fomichev <sdf@google.com>
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
>
> ---
> * 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
fwiw I checked and the perf UNLOAD events have proper id now
thanks for fixing this
jirka
> * 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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD
2023-01-10 9:10 ` Jiri Olsa
@ 2023-01-10 16:55 ` Paul Moore
0 siblings, 0 replies; 12+ messages in thread
From: Paul Moore @ 2023-01-10 16:55 UTC (permalink / raw)
To: Jiri Olsa
Cc: linux-audit, bpf, Burn Alting, Stanislav Fomichev, Alexei Starovoitov
On Tue, Jan 10, 2023 at 4:10 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> On Fri, Jan 06, 2023 at 10:43:59AM -0500, Paul Moore 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@vger.kernel.org
> > Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq context.")
> > Reported-by: Burn Alting <burn.alting@iinet.net.au>
> > Reported-by: Jiri Olsa <olsajiri@gmail.com>
> > Suggested-by: Stanislav Fomichev <sdf@google.com>
> > Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> >
> > ---
> > * 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
>
> fwiw I checked and the perf UNLOAD events have proper id now
> thanks for fixing this
No problem, thanks for verifying that this solves the perf problem too.
--
paul-moore.com
^ permalink raw reply [flat|nested] 12+ messages in thread