Re: [PATCH bpf-next v13 1/5] bpf: Move constants blinding out of arch-specific JITs

[bot+bpf-ci]

> leaving the global env->prog->insni and env->insn_aux_data untouched.

The struct field is insnsi, not insni. The same paragraph also says:

> this approach result in incorrect index for the insni array

Should this be "results in" and "insnsi"?

The commit message also refers to clean_insn_aux_data():

> it is freed by clean_insn_aux_data() at the end of bpf_check().
> Before freeing, clean_insn_aux_data() loops over env->insn_aux_data

The function is actually clear_insn_aux_data(), not
clean_insn_aux_data(). Searching the tree for clean_insn_aux_data
returns no results.

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 066b86e7233c..3fa3c74869d0 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1508,7 +1508,10 @@ static void adjust_insn_arrays(struct bpf_prog *prog, u32 off, u32 len)
>  #endif
>  }
>
> -struct bpf_prog *bpf_jit_blind_constants(struct bpf_prog *prog)
> +/* Now this function is used only to blind the main prog and must be invoked only when
> + * bpf_prog_need_blind() returns true.
> + */
> +struct bpf_prog *bpf_jit_blind_constants(struct bpf_verifier_env *env, struct bpf_prog *prog)

The BPF subsystem convention is for multi-line comments to have the
opening /* on its own line. Should this be:

/*
 * Now this function is used only to blind the main prog and must be
 * invoked only when bpf_prog_need_blind() returns true.
 */

[ ... ]

> @@ -1550,21 +1557,34 @@ struct bpf_prog *bpf_jit_blind_constants(struct bpf_prog *prog)

[ ... ]

> +		if (env)
> +			env->prog = clone;
> +		else
> +			/* Instructions arrays must be updated using absolute xlated offsets.
> +			 * The arrays have already been adjusted by bpf_patch_insn_data() when
> +			 * env is not NULL.
> +			 */
> +			adjust_insn_arrays(clone, i, rewritten);

Same comment style question here.

[ ... ]

> @@ -2533,6 +2553,34 @@ static bool bpf_prog_select_interpreter(struct bpf_prog *fp)

[ ... ]

> +	prog = bpf_jit_blind_constants(NULL, prog);
> +	/* If blinding was requested and we failed during blinding, we must fall
> +	 * back to the interpreter.
> +	 */
> +	if (IS_ERR(prog))

And here as well.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 56fcc96dc780..0cca590915ff 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> +	} else {
> +		if (err != -EFAULT) {
> +			/* We will fall back to interpreter mode when err is not -EFAULT, before
> +			 * that, insn->off and insn->imm should be restored to their original
> +			 * values since they were modified by __jit_subprogs.
> +			 */

Same comment style question.


---
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/24284268460