Re: [PATCH bpf-next v3 2/7] bpf: Refactor {acquire,release}_reference_state

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2024-11-27 at 08:58 -0800, Kumar Kartikeya Dwivedi wrote:

Overall looks good, but please take a look at a few notes below.

[...]

> @@ -1349,77 +1350,69 @@ static int grow_stack_state(struct bpf_verifier_env *env, struct bpf_func_state
>   * On success, returns a valid pointer id to associate with the register
>   * On failure, returns a negative errno.
>   */
> -static int acquire_reference_state(struct bpf_verifier_env *env, int insn_idx)
> +static struct bpf_reference_state *acquire_reference_state(struct bpf_verifier_env *env, int insn_idx, bool gen_id)
>  {
>  	struct bpf_verifier_state *state = env->cur_state;
>  	int new_ofs = state->acquired_refs;
> -	int id, err;
> +	int err;
>  
>  	err = resize_reference_state(state, state->acquired_refs + 1);
>  	if (err)
> -		return err;
> -	id = ++env->id_gen;
> -	state->refs[new_ofs].type = REF_TYPE_PTR;
> -	state->refs[new_ofs].id = id;
> +		return NULL;
> +	if (gen_id)
> +		state->refs[new_ofs].id = ++env->id_gen;

Nit: state->refs[new_ods].id might end up with garbage value if 'gen_id' is false.
     The resize_reference_state() uses realloc_array(),
     which allocates memory with GFP_KERNEL, but without __GFP_ZERO flag.
     This is not a problem with current patch, as you always check
     reference type before checking id, but most of the data strucures
     in verifier are zero initialized just in case.

>  	state->refs[new_ofs].insn_idx = insn_idx;
>  
> -	return id;
> +	return &state->refs[new_ofs];
> +}

[...]

> -/* release function corresponding to acquire_reference_state(). Idempotent. */
> -static int release_reference_state(struct bpf_verifier_state *state, int ptr_id)
> +static void release_reference_state(struct bpf_verifier_state *state, int idx)
>  {
> -	int i, last_idx;
> +	int last_idx;
>  
>  	last_idx = state->acquired_refs - 1;
> -	for (i = 0; i < state->acquired_refs; i++) {
> -		if (state->refs[i].type != REF_TYPE_PTR)
> -			continue;
> -		if (state->refs[i].id == ptr_id) {
> -			if (last_idx && i != last_idx)
> -				memcpy(&state->refs[i], &state->refs[last_idx],
> -				       sizeof(*state->refs));
> -			memset(&state->refs[last_idx], 0, sizeof(*state->refs));
> -			state->acquired_refs--;
> -			return 0;
> -		}
> -	}
> -	return -EINVAL;
> +	if (last_idx && idx != last_idx)
> +		memcpy(&state->refs[idx], &state->refs[last_idx], sizeof(*state->refs));
> +	memset(&state->refs[last_idx], 0, sizeof(*state->refs));
> +	state->acquired_refs--;
> +	return;
>  }

Such implementation replaces element at 'idx' with element at 'last_idx'.
If the intention is to use 'state->refs' as a stack of acquired irq flags,
the stack property would be broken by this trick.
E.g. consider array [a, b, c, d] where 'idx' points to 'b',
after release_reference_state() the array would become [a, d, c].
You need to do 'memmove' instead.

[...]

> @@ -9666,21 +9659,41 @@ static void mark_pkt_end(struct bpf_verifier_state *vstate, int regn, bool range
>  		reg->range = AT_PKT_END;
>  }
>  
> +static int release_reference_nomark(struct bpf_verifier_state *state, int ref_obj_id)
> +{
> +	int i;
> +
> +	for (i = 0; i < state->acquired_refs; i++) {
> +		if (state->refs[i].type != REF_TYPE_PTR)
> +			continue;
> +		if (state->refs[i].id == ref_obj_id) {
> +			release_reference_state(state, i);
> +			return 0;
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
>  /* The pointer with the specified id has released its reference to kernel
>   * resources. Identify all copies of the same pointer and clear the reference.
> + *
> + * This is the release function corresponding to acquire_reference(). Idempotent.
> + * The 'mark' boolean is used to optionally skip scrubbing registers matching
          ^^^^^^
Nit: this is probably a remnant of some older patch revision,
     function no longer takes 'mark' parameter.

> + * the ref_obj_id, in case they need to be switched to some other type instead
> + * of havoc scalar value.
>   */
> -static int release_reference(struct bpf_verifier_env *env,
> -			     int ref_obj_id)
> +static int release_reference(struct bpf_verifier_env *env, int ref_obj_id)
>  {

[...]