On Sat, 2025-08-16 at 18:06 +0000, Anton Protopopov wrote:
The change makes sense to me, please see a few comments below.
[...]
> @@ -2111,12 +2111,12 @@ static struct bpf_verifier_state *push_stack(struct bpf_verifier_env *env,
> env->stack_size++;
> err = copy_verifier_state(&elem->st, cur);
> if (err)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
> elem->st.speculative |= speculative;
> if (env->stack_size > BPF_COMPLEXITY_LIMIT_JMP_SEQ) {
> verbose(env, "The sequence of %d jumps is too complex.\n",
> env->stack_size);
> - return NULL;
> + return ERR_PTR(-EFAULT);
Nit: this should be -E2BIG, I think.
> }
> if (elem->st.parent) {
> ++elem->st.parent->branches;
> @@ -2912,7 +2912,7 @@ static struct bpf_verifier_state *push_async_cb(struct bpf_verifier_env *env,
>
> elem = kzalloc(sizeof(struct bpf_verifier_stack_elem), GFP_KERNEL_ACCOUNT);
> if (!elem)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> elem->insn_idx = insn_idx;
> elem->prev_insn_idx = prev_insn_idx;
> @@ -2924,7 +2924,7 @@ static struct bpf_verifier_state *push_async_cb(struct bpf_verifier_env *env,
> verbose(env,
> "The sequence of %d jumps is too complex for async cb.\n",
> env->stack_size);
> - return NULL;
> + return ERR_PTR(-EFAULT);
(and here too)
> }
> /* Unlike push_stack() do not copy_verifier_state().
> * The caller state doesn't matter.
[...]
> @@ -14217,7 +14217,7 @@ sanitize_speculative_path(struct bpf_verifier_env *env,
> struct bpf_reg_state *regs;
>
> branch = push_stack(env, next_idx, curr_idx, true);
> - if (branch && insn) {
> + if (!IS_ERR(branch) && insn) {
Note: branch returned by `sanitize_speculative_path` is never used.
Maybe change the function to return `int` and do the regular
err = sanitize_speculative_path()
if (err)
return err;
thing here?
> regs = branch->frame[branch->curframe]->regs;
> if (BPF_SRC(insn->code) == BPF_K) {
> mark_reg_unknown(env, regs, insn->dst_reg);
[...]
> @@ -16721,8 +16720,7 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
> * execution.
> */
> if (!env->bypass_spec_v1 &&
> - !sanitize_speculative_path(env, insn, *insn_idx + 1,
> - *insn_idx))
> + IS_ERR(sanitize_speculative_path(env, insn, *insn_idx + 1, *insn_idx)))
> return -EFAULT;
I think the error code should be taken from the return value of the
sanitize_speculative_path().
> if (env->log.level & BPF_LOG_LEVEL)
> print_insn_state(env, this_branch, this_branch->curframe);
> @@ -16734,9 +16732,9 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
> * simulation under speculative execution.
> */
> if (!env->bypass_spec_v1 &&
> - !sanitize_speculative_path(env, insn,
> - *insn_idx + insn->off + 1,
> - *insn_idx))
> + IS_ERR(sanitize_speculative_path(env, insn,
> + *insn_idx + insn->off + 1,
> + *insn_idx)))
Same here.
> return -EFAULT;
> if (env->log.level & BPF_LOG_LEVEL)
> print_insn_state(env, this_branch, this_branch->curframe);
[...]
