Re: [PATCH v1 bpf-next 01/11] bpf: fix the return value of push_stack

[Eduard Zingerman <eddyz87@gmail.com>]

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);

[...]