From: Stanislav Fomichev <stfomichev@yandex.ru> To: alexei.starovoitov@gmail.com Cc: andrii@kernel.org, ast@kernel.org, bpf@vger.kernel.org, burn.alting@iinet.net.au, daniel@iogearbox.net, jolsa@kernel.org, linux-audit@redhat.com, paul@paul-moore.com, sdf@google.com, stfomichev@yandex.ru Subject: Re: [PATCH v2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD Date: Tue, 27 Dec 2022 16:25:02 -0800 [thread overview] Message-ID: <20221228002502.3972381-1-stfomichev@yandex.ru> (raw) In-Reply-To: <CAADnVQJSTXOfsEunq=z+8tiQPSHpDmtXX3eP9QcM2QvBefgaag@mail.gmail.com> > On Mon, Dec 26, 2022 at 7:35 PM Stanislav Fomichev <stfomichev@yandex.ru> wrote: > > > > > On Fri, Dec 23, 2022 at 5:49 PM Stanislav Fomichev <sdf@google.com> wrote: > > > get_func_ip() */ > > > > > - tstamp_type_access:1; /* Accessed __sk_buff->tstamp_type */ > > > > > + tstamp_type_access:1, /* Accessed __sk_buff->tstamp_type */ > > > > > + valid_id:1; /* Is bpf_prog::aux::__id valid? */ > > > > > enum bpf_prog_type type; /* Type of BPF program */ > > > > > enum bpf_attach_type expected_attach_type; /* For some prog types */ > > > > > u32 len; /* Number of filter blocks */ > > > > > @@ -1688,6 +1689,12 @@ 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); > > > > > > > > > > +static inline u32 bpf_prog_get_id(const struct bpf_prog *prog) > > > > > +{ > > > > > + if (WARN(!prog->valid_id, "Attempting to use an invalid eBPF program")) > > > > > + return 0; > > > > > + return prog->aux->__id; > > > > > +} > > > > > > > > I'm still missing why we need to have this WARN and have a check at all. > > > > IIUC, we're actually too eager in resetting the id to 0, and need to > > > > keep that stale id around at least for perf/audit. > > > > Why not have a flag only to protect against double-idr_remove > > > > bpf_prog_free_id and keep the rest as is? > > > > Which places are we concerned about that used to report id=0 but now > > > > would report stale id? > > > > > > What double-idr_remove are you concerned about? > > > bpf_prog_by_id() is doing bpf_prog_inc_not_zero > > > while __bpf_prog_put just dropped it to zero. > > > > (traveling, sending from an untested setup, hope it reaches everyone) > > > > There is a call to bpf_prog_free_id from __bpf_prog_offload_destroy which > > tries to make offloaded program disappear from the idr when the netdev > > goes offline. So I'm assuming that '!prog->aux->id' check in bpf_prog_free_id > > is to handle that case where we do bpf_prog_free_id much earlier than the > > rest of the __bpf_prog_put stuff. > > That remove was done in > commit ad8ad79f4f60 ("bpf: offload: free program id when device disappears") > Back in 2017 there was no bpf audit and no > PERF_BPF_EVENT_PROG_LOAD/UNLOAD events. > > The removal of id made sense back then to avoid showing this > 'useless' orphaned offloaded prog in 'bpftool prog show', > but with addition of perf load/unload and audit it was no longer > correct to zero out ID in a prog which refcnt is still not zero. > > So we should simply remove bpf_prog_free_id from __bpf_prog_offload_destroy. > There won't be any adverse effect other than bpftool prog show > will show orphaned progs. SGTM, that would simplify everything.. > > > > > Maybe just move bpf_prog_free_id() into bpf_prog_put_deferred() > > > after perf_event_bpf_event and bpf_audit_prog ? > > > Probably can remove the obsolete do_idr_lock bool flag as > > > separate patch? > > > > +1 on removing do_idr_lock separately. > > > > > Much simpler fix and no code churn. > > > Both valid_id and saved_id approaches have flaws. > > > > Given the __bpf_prog_offload_destroy path above, we still probably need > > some flag to indicate that the id has been already removed from the idr? > > No. ID should be valid until prog went through perf and audit unload > events. Don't know about audit, but for perf it's essential to have > valid ID otherwise perf record will not be able to properly associate events.
WARNING: multiple messages have this Message-ID (diff)
From: Stanislav Fomichev <stfomichev@yandex.ru> To: alexei.starovoitov@gmail.com Cc: burn.alting@iinet.net.au, daniel@iogearbox.net, andrii@kernel.org, ast@kernel.org, linux-audit@redhat.com, sdf@google.com, jolsa@kernel.org, bpf@vger.kernel.org, stfomichev@yandex.ru Subject: Re: [PATCH v2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD Date: Tue, 27 Dec 2022 16:25:02 -0800 [thread overview] Message-ID: <20221228002502.3972381-1-stfomichev@yandex.ru> (raw) In-Reply-To: <CAADnVQJSTXOfsEunq=z+8tiQPSHpDmtXX3eP9QcM2QvBefgaag@mail.gmail.com> > On Mon, Dec 26, 2022 at 7:35 PM Stanislav Fomichev <stfomichev@yandex.ru> wrote: > > > > > On Fri, Dec 23, 2022 at 5:49 PM Stanislav Fomichev <sdf@google.com> wrote: > > > get_func_ip() */ > > > > > - tstamp_type_access:1; /* Accessed __sk_buff->tstamp_type */ > > > > > + tstamp_type_access:1, /* Accessed __sk_buff->tstamp_type */ > > > > > + valid_id:1; /* Is bpf_prog::aux::__id valid? */ > > > > > enum bpf_prog_type type; /* Type of BPF program */ > > > > > enum bpf_attach_type expected_attach_type; /* For some prog types */ > > > > > u32 len; /* Number of filter blocks */ > > > > > @@ -1688,6 +1689,12 @@ 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); > > > > > > > > > > +static inline u32 bpf_prog_get_id(const struct bpf_prog *prog) > > > > > +{ > > > > > + if (WARN(!prog->valid_id, "Attempting to use an invalid eBPF program")) > > > > > + return 0; > > > > > + return prog->aux->__id; > > > > > +} > > > > > > > > I'm still missing why we need to have this WARN and have a check at all. > > > > IIUC, we're actually too eager in resetting the id to 0, and need to > > > > keep that stale id around at least for perf/audit. > > > > Why not have a flag only to protect against double-idr_remove > > > > bpf_prog_free_id and keep the rest as is? > > > > Which places are we concerned about that used to report id=0 but now > > > > would report stale id? > > > > > > What double-idr_remove are you concerned about? > > > bpf_prog_by_id() is doing bpf_prog_inc_not_zero > > > while __bpf_prog_put just dropped it to zero. > > > > (traveling, sending from an untested setup, hope it reaches everyone) > > > > There is a call to bpf_prog_free_id from __bpf_prog_offload_destroy which > > tries to make offloaded program disappear from the idr when the netdev > > goes offline. So I'm assuming that '!prog->aux->id' check in bpf_prog_free_id > > is to handle that case where we do bpf_prog_free_id much earlier than the > > rest of the __bpf_prog_put stuff. > > That remove was done in > commit ad8ad79f4f60 ("bpf: offload: free program id when device disappears") > Back in 2017 there was no bpf audit and no > PERF_BPF_EVENT_PROG_LOAD/UNLOAD events. > > The removal of id made sense back then to avoid showing this > 'useless' orphaned offloaded prog in 'bpftool prog show', > but with addition of perf load/unload and audit it was no longer > correct to zero out ID in a prog which refcnt is still not zero. > > So we should simply remove bpf_prog_free_id from __bpf_prog_offload_destroy. > There won't be any adverse effect other than bpftool prog show > will show orphaned progs. SGTM, that would simplify everything.. > > > > > Maybe just move bpf_prog_free_id() into bpf_prog_put_deferred() > > > after perf_event_bpf_event and bpf_audit_prog ? > > > Probably can remove the obsolete do_idr_lock bool flag as > > > separate patch? > > > > +1 on removing do_idr_lock separately. > > > > > Much simpler fix and no code churn. > > > Both valid_id and saved_id approaches have flaws. > > > > Given the __bpf_prog_offload_destroy path above, we still probably need > > some flag to indicate that the id has been already removed from the idr? > > No. ID should be valid until prog went through perf and audit unload > events. Don't know about audit, but for perf it's essential to have > valid ID otherwise perf record will not be able to properly associate events. -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
next prev parent reply other threads:[~2022-12-28 0:31 UTC|newest] Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-12-23 18:55 [PATCH v2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD Paul Moore 2022-12-23 18:55 ` Paul Moore 2022-12-23 21:14 ` kernel test robot 2022-12-23 21:14 ` kernel test robot 2022-12-23 21:26 ` Paul Moore 2022-12-23 21:26 ` Paul Moore 2022-12-23 21:44 ` kernel test robot 2022-12-23 21:44 ` kernel test robot 2022-12-24 1:49 ` Stanislav Fomichev 2022-12-24 1:49 ` Stanislav Fomichev 2022-12-24 15:31 ` Paul Moore 2022-12-24 15:31 ` Paul Moore 2022-12-25 22:16 ` Alexei Starovoitov 2022-12-25 22:16 ` Alexei Starovoitov 2022-12-27 3:35 ` Stanislav Fomichev 2022-12-27 3:35 ` Stanislav Fomichev 2022-12-27 16:40 ` Paul Moore 2022-12-27 16:40 ` Paul Moore 2022-12-30 2:13 ` Stanislav Fomichev 2022-12-30 2:13 ` Stanislav Fomichev 2022-12-30 3:10 ` Alexei Starovoitov 2022-12-30 3:10 ` Alexei Starovoitov 2022-12-30 3:38 ` Stanislav Fomichev 2022-12-30 3:38 ` Stanislav Fomichev 2022-12-30 4:18 ` Alexei Starovoitov 2022-12-30 4:18 ` Alexei Starovoitov 2022-12-27 17:49 ` Alexei Starovoitov 2022-12-27 17:49 ` Alexei Starovoitov 2022-12-28 0:25 ` Stanislav Fomichev [this message] 2022-12-28 0:25 ` Stanislav Fomichev 2022-12-25 14:13 ` Jiri Olsa 2022-12-25 14:13 ` Jiri Olsa 2022-12-25 19:14 ` Paul Moore 2022-12-25 19:14 ` Paul Moore 2023-01-03 0:33 ` kernel test robot 2023-01-03 0:33 ` kernel test robot 2023-01-03 18:12 Bryce
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20221228002502.3972381-1-stfomichev@yandex.ru \ --to=stfomichev@yandex.ru \ --cc=alexei.starovoitov@gmail.com \ --cc=andrii@kernel.org \ --cc=ast@kernel.org \ --cc=bpf@vger.kernel.org \ --cc=burn.alting@iinet.net.au \ --cc=daniel@iogearbox.net \ --cc=jolsa@kernel.org \ --cc=linux-audit@redhat.com \ --cc=paul@paul-moore.com \ --cc=sdf@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.