Re: [PATCH bpf-next v1 1/7] bpf: Refactor and rename resource management

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2024-11-20 at 16:53 -0800, Kumar Kartikeya Dwivedi wrote:
> With the commit f6b9a69a9e56 ("bpf: Refactor active lock management"),
> we have begun using the acquired_refs array to also store active lock
> metadata, as a way to consolidate and manage all kernel resources that
> the program may acquire.
> 
> This is beginning to cause some confusion and duplication in existing
> code, where the terms references now both mean lock reference state and
> the references for acquired kernel object pointers. To clarify and
> improve the current state of affairs, as well as reduce code duplication,
> make the following changes:
> 
> Rename bpf_reference_state to bpf_resource_state, and begin using
> resource as the umbrella term. This terminology matches what we use in
> check_resource_leak. Next, "reference" now only means RES_TYPE_PTR, and
> the usage and meaning is updated accordingly.
> 
> Next, factor out common code paths for managing addition and removal of
> resource state in acquire_resource_state and erase_resource_state, and
> then implement type specific resource handling on top of these common
> functions. Overall, this patch improves upon the confusion and minimizes
> code duplication, as we prepare to introduce new resource types in
> subsequent patches.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

Tbh, I like the old name a bit more.
The patch itself looks good.

Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> @@ -1342,6 +1342,25 @@ static int grow_stack_state(struct bpf_verifier_env *env, struct bpf_func_state
>  	return 0;
>  }
>  
> +static struct bpf_resource_state *acquire_resource_state(struct bpf_verifier_env *env, int insn_idx, int *id)

Nit: there is no need to pass `int *id`, as it is available as (returned)->id.

> +{
> +	struct bpf_func_state *state = cur_func(env);
> +	int new_ofs = state->acquired_res;
> +	struct bpf_resource_state *s;
> +	int err;
> +
> +	err = resize_resource_state(state, state->acquired_res + 1);
> +	if (err)
> +		return NULL;
> +	s = &state->res[new_ofs];
> +	s->type = RES_TYPE_INV;
> +	if (id)
> +		*id = s->id = ++env->id_gen;
> +	s->insn_idx = insn_idx;
> +
> +	return s;
> +}
> +
>  /* Acquire a pointer id from the env and update the state->refs to include
>   * this new pointer reference.
>   * On success, returns a valid pointer id to associate with the register

[...]

> @@ -1349,55 +1368,52 @@ static int grow_stack_state(struct bpf_verifier_env *env, struct bpf_func_state

[...]

> -/* release function corresponding to acquire_reference_state(). Idempotent. */
> +static void erase_resource_state(struct bpf_func_state *state, int res_idx)

Nit: why not "release_..." to be consistent with the rest of the functions?

> +{
> +	int last_idx = state->acquired_res - 1;
> +
> +	if (last_idx && res_idx != last_idx)
> +		memcpy(&state->res[res_idx], &state->res[last_idx], sizeof(*state->res));
> +	memset(&state->res[last_idx], 0, sizeof(*state->res));
> +	state->acquired_res--;
> +}
> +
>  static int release_reference_state(struct bpf_func_state *state, int ptr_id)
>  {
> -	int i, last_idx;
> +	int i;
>  
> -	last_idx = state->acquired_refs - 1;
> -	for (i = 0; i < state->acquired_refs; i++) {
> -		if (state->refs[i].type != REF_TYPE_PTR)
> +	for (i = 0; i < state->acquired_res; i++) {
> +		if (state->res[i].type != RES_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--;
> +		if (state->res[i].id == ptr_id) {
> +			erase_resource_state(state, i);
>  			return 0;
>  		}
>  	}

[...]