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@xxxxxxxxx>
> ---
Tbh, I like the old name a bit more.
The patch itself looks good.
Reviewed-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
[...]
> @@ -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;
> }
> }
[...]
