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

[sdf@google.com]

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?

> Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq  
> context.")
> Reported-by: Burn Alting <burn.alting@iinet.net.au>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>   include/linux/bpf.h  | 1 +
>   kernel/bpf/syscall.c | 8 +++++---
>   2 files changed, 6 insertions(+), 3 deletions(-)

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 9e7d46d16032..a22001ceb2c3 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1103,6 +1103,7 @@ struct bpf_prog_aux {
>   	u32 max_tp_access;
>   	u32 stack_depth;
>   	u32 id;
> +	u32 id_audit; /* preserves the id for use by audit */
>   	u32 func_cnt; /* used by non-func prog as the number of func progs */
>   	u32 func_idx; /* 0 for non-func prog, the index in func array for func  
> prog */
>   	u32 attach_btf_id; /* in-kernel BTF type id to attach to */
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 7b373a5e861f..3ec09f4dba18 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1958,13 +1958,13 @@ 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))
>   		return;
>   	audit_log_format(ab, "prog-id=%u op=%s",
> -			 prog->aux->id, bpf_audit_str[op]);
> +			 prog->aux->id_audit, bpf_audit_str[op]);
>   	audit_log_end(ab);
>   }

> @@ -1975,8 +1975,10 @@ static int bpf_prog_alloc_id(struct bpf_prog *prog)
>   	idr_preload(GFP_KERNEL);
>   	spin_lock_bh(&prog_idr_lock);
>   	id = idr_alloc_cyclic(&prog_idr, prog, 1, INT_MAX, GFP_ATOMIC);
> -	if (id > 0)
> +	if (id > 0) {
>   		prog->aux->id = id;
> +		prog->aux->id_audit = id;
> +	}
>   	spin_unlock_bh(&prog_idr_lock);
>   	idr_preload_end();

> --
> 2.39.0