Re: [PATCH bpf-next 1/5] bpf: exact states comparison for iterator convergence checks

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Sat, Oct 21, 2023 at 03:59:35AM +0300, Eduard Zingerman wrote:
>  
> +static struct bpf_verifier_state_list **__explored_state(struct bpf_verifier_env *env,
> +							 int idx,
> +							 int callsite);
...
> +static struct bpf_verifier_state *find_prev_entry(struct bpf_verifier_env *env,
> +						  struct bpf_verifier_state *cur,
> +						  int insn_idx)
> +{
> +	struct bpf_verifier_state_list *sl;
> +	struct bpf_verifier_state *st;
> +
> +	/* Explored states are pushed in stack order, most recent states come first */
> +	sl = *__explored_state(env, insn_idx, cur->frame[cur->curframe]->callsite);
...
> +		prev_st = find_prev_entry(env, cur_st->parent, insn_idx);
...
> +static struct bpf_verifier_state_list **__explored_state(struct bpf_verifier_env *env,
> +							 int idx,
> +							 int callsite)
> +{
> +	return &env->explored_states[(idx ^ callsite) % state_htab_size(env)];
> +}
> +
>  static struct bpf_verifier_state_list **explored_state(
>  					struct bpf_verifier_env *env,
>  					int idx)
> @@ -15032,7 +15161,7 @@ static struct bpf_verifier_state_list **explored_state(
>  	struct bpf_verifier_state *cur = env->cur_state;
>  	struct bpf_func_state *state = cur->frame[cur->curframe];
>  
> -	return &env->explored_states[(idx ^ state->callsite) % state_htab_size(env)];
> +	return __explored_state(env, idx, state->callsite);
>  }

Do we really need to introduce this new helper?
I suspect the concern was that cur->callsite != cur->parent->callsite, right?
But that can never be the case, since bpf_iter_num_next() is force checkpoint,
so inside process_iter_next_call() cur_st->parent is guaranteed to be from
the same function and callsites will be the same.
I can undo above and replace with a warn_on (or with a comment) while applying?