Re: [PATCH bpf-next 1/8] bpf: Save pruning point states in oracle

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3256 bytes --]

> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 130bcbd66f60..adaeff35aaa6 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h

[ ... ]

> @@ -551,6 +575,7 @@ struct bpf_insn_aux_data {
>  	};
>  	struct bpf_iarray *jt;	/* jump table for gotox or bpf_tailcall call instruction */
>  	struct btf_struct_meta *kptr_struct_meta;
> +	struct list_head *oracle_states;
>  	u64 map_key_state; /* constant (32 bit) key tracking for maps */
>  	int ctx_field_size; /* the ctx field size for load insn, maybe 0 */
>  	u32 seen; /* this insn was processed by the verifier at env->pass_cnt */

[ ... ]

> diff --git a/kernel/bpf/oracle.c b/kernel/bpf/oracle.c
> new file mode 100644
> index 000000000000..adbb153aadee
> --- /dev/null
> +++ b/kernel/bpf/oracle.c
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * This file implements a test oracle for the verifier. When the oracle is enabled, the verifier
> + * saves information on variables at regular points throughout the program. This information is
> + * then compared at runtime with the concrete values to ensure that the verifier's information is
> + * correct.
> + */
> +
> +#include <linux/bpf_verifier.h>
> +
> +static void convert_oracle_state(struct bpf_verifier_state *istate, struct bpf_oracle_state *ostate)
> +{

[ ... ]

> +int save_state_in_oracle(struct bpf_verifier_env *env, int insn_idx)
> +{
> +	struct bpf_verifier_state *cur = env->cur_state;
> +	struct bpf_insn_aux_data *aux = cur_aux(env);
> +	struct bpf_oracle_state_list *new_sl;
> +
> +	if (env->subprog_cnt > 1)
> +		/* Skip the oracle if subprogs are used. */
> +		return 0;
> +
> +	if (!aux->oracle_states) {
> +		aux->oracle_states = kmalloc(sizeof(*aux->oracle_states), GFP_KERNEL_ACCOUNT);
> +		if (!aux->oracle_states)
> +			return -ENOMEM;
> +
> +		INIT_LIST_HEAD(aux->oracle_states);
> +	}
> +
> +	new_sl = kzalloc(sizeof(*new_sl), GFP_KERNEL_ACCOUNT);
> +	if (!new_sl)
> +		return -ENOMEM;

Can this leak the oracle_states list_head allocated above? If the
kmalloc() succeeds but this kzalloc() fails, save_state_in_oracle()
returns -ENOMEM without freeing aux->oracle_states. Looking at
do_check()->save_state_in_oracle() error handling, the error propagates
up and the verifier fails, but aux->oracle_states remains allocated.

> +	convert_oracle_state(cur, &new_sl->state);
> +	list_add(&new_sl->node, aux->oracle_states);
> +
> +	return 0;
> +}

Also, where is the cleanup code for oracle_states? Looking at
clear_insn_aux_data() in verifier.c, it only frees the jt field:

    for (i = start; i < end; i++) {
        if (aux_data[i].jt) {
            kvfree(aux_data[i].jt);
            aux_data[i].jt = NULL;
        }
        ...
    }

Should clear_insn_aux_data() also walk through aux->oracle_states,
freeing each bpf_oracle_state_list node, then free the list_head itself?
Without this, both the list_head and all oracle_state_list nodes will
leak on every BPF program verification.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/20014308092