On 25/08/25 11:12AM, Eduard Zingerman wrote:
> 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.
I didn't want to change the set of return values. Agree that the
-E2BIG error looks better here, plus there's a corresponding verifier
message, so I will resend with -E2BIG.
Also agree with all your comments below, will address them in v2.
> > }
> > 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);
>
> [...]
