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

[Anton Protopopov <a.s.protopopov@gmail.com>]

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);
> 
> [...]