Re: [PATCH] bpf: restore the ebpf audit UNLOAD id field

On Fri, Dec 23, 2022 at 10:37 AM Paul Moore <paul(a)paul-moore.com&gt; wrote: > On Thu, Dec 22, 2022 at 6:20 PM Jiri Olsa <olsajiri(a)gmail.com&gt; wrote: > > On Thu, Dec 22, 2022 at 02:03:41PM -0500, Paul Moore wrote: > > > On Thu, Dec 22, 2022 at 12:19 PM <sdf(a)google.com&gt; 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.

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,

}, }; > 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.