From: Jiri Olsa <olsajiri@gmail.com>
To: Paul Moore <paul@paul-moore.com>
Cc: Burn Alting <burn.alting@iinet.net.au>,
Alexei Starovoitov <ast@kernel.org>,
linux-audit@redhat.com, sdf@google.com, bpf@vger.kernel.org,
Jiri Olsa <olsajiri@gmail.com>
Subject: Re: [PATCH] bpf: restore the ebpf audit UNLOAD id field
Date: Fri, 23 Dec 2022 19:03:56 +0100 [thread overview]
Message-ID: <Y6XtjK49rM44YI/Q@krava> (raw)
In-Reply-To: <CAHC9VhR2Q1SiCFoJnqr--W-cuTwpreio0knYkRcHw2MEd06rHA@mail.gmail.com>
On Fri, Dec 23, 2022 at 10:58:37AM -0500, Paul Moore wrote:
> On Fri, Dec 23, 2022 at 10:37 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Dec 22, 2022 at 6:20 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> > > On Thu, Dec 22, 2022 at 02:03:41PM -0500, Paul Moore wrote:
> > > > On Thu, Dec 22, 2022 at 12:19 PM <sdf@google.com> wrote:
> > > > > On 12/21, 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
> > > > > > generating the audit UNLOAD record, which obviously rendered the ID
> > > > > > field bogus (always zero). This patch resolves this by adding a new
> > > > > > field, bpf_prog_aux::id_audit, which is set when the ebpf program is
> > > > > > allocated an ID and never reset, ensuring a valid ID field,
> > > > > > regardless of the state of the original ID field, bpf_prox_aud::id.
> > > > >
> > > > > > 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).
> > > > >
> > > > > [..]
> > > > >
> > > > > > As an note to future bug hunters, I did briefly consider removing the
> > > > > > ID reset in bpf_prog_free_id(), as it would seem that once the
> > > > > > program is removed from the idr pool it can no longer be found by its
> > > > > > ID value, but commit ad8ad79f4f60 ("bpf: offload: free program id
> > > > > > when device disappears") seems to imply that it is beneficial to
> > > > > > reset the ID value. Perhaps as a secondary indicator that the ebpf
> > > > > > program is unbound/orphaned.
> > > > >
> > > > > That seems like the way to go imho. Can we have some extra 'invalid_id'
> > > > > bitfield in the bpf_prog so we can set it in bpf_prog_free_id and
> > > > > check in bpf_prog_free_id (for this offloaded use-case)? Because
> > > > > having two ids and then keeping track about which one to use, depending
> > > > > on the context, seems more fragile?
> > > >
> > > > I would definitely prefer to keep just a single ID value, and that was
> > > > the first approach I took when drafting this patch, but when looking
> > > > through the git log it looked like there was some desire to reset the
> > > > ID to zero on free. Not being an expert on the ebpf kernel code I
> > > > figured I would just write the patch up this way and make a comment
> > > > about not zero'ing out the ID in the commit description so we could
> > > > have a discussion about it.
> > > >
> > > > I'm not seeing any other comments, so I'll go ahead with putting
> > > > together a v2 that sets an invalid flag/bit and I'll post that for
> > > > further discussion/review.
> > >
> > > great, perf suffers the same issue:
> > > https://lore.kernel.org/bpf/Y3SRWVoycV290S16@krava/
> > >
> > > any chance you could include it as well? I can send a patch
> > > later if needed
> >
> > Hi Jiri,
> >
> > I'm pretty sure the current approach recommended by Stanislav, to
> > never reset/zero the ID and instead mark it as invalid via a flag in
> > the bpf_prog struct, should resolve the perf problem as well.
ok, I misunderstood
>
> I probably should elaborate on this a bit more, in the case of
> perf_event_bpf_event() the getter which checks the valid_id flag isn't
> used, rather a direct access of bpf_prog_aux::__id is done so that the
> ID can be retrieved even after it is free'd/marked-invalid. Here is
> the relevant code snippet for the patch:
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index aefc1e08e015..c24e897d27f1 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -9001,7 +9001,11 @@ void perf_event_bpf_event(struct bpf_prog *prog,
> },
> .type = type,
> .flags = flags,
> - .id = prog->aux->id,
> + /*
> + * don't use bpf_prog_get_id() as the id may be marked
> + * invalid on PERF_BPF_EVENT_PROG_UNLOAD events
> + */
> + .id = prog->aux->__id,
looks good
> },
> };
>
> > My time
> > is a little short at the moment due to the holidays, but perhaps with
> > a little luck I'll get a new revision of the patch posted soon
> > (today?) and you can take a look and give it a test. Are you
> > subscribed to the linux-audit and/or bpf mailing lists? If not I can
> > CC you directly on the next revision.
bpf list is fine
thanks,
jirka
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
prev parent reply other threads:[~2022-12-23 18:07 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-22 0:13 [PATCH] bpf: restore the ebpf audit UNLOAD id field Paul Moore
2022-12-22 17:19 ` sdf
2022-12-22 19:03 ` Paul Moore
2022-12-22 19:40 ` sdf
2022-12-22 19:59 ` Paul Moore
2022-12-22 20:07 ` Paul Moore
2022-12-22 21:27 ` Stanislav Fomichev
2022-12-23 15:30 ` Paul Moore
2022-12-22 23:20 ` Jiri Olsa
2022-12-23 15:37 ` Paul Moore
2022-12-23 15:58 ` Paul Moore
2022-12-23 18:03 ` Jiri Olsa [this message]
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=Y6XtjK49rM44YI/Q@krava \
--to=olsajiri@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=burn.alting@iinet.net.au \
--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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).