On Thu, 21 Nov 2024 at 17:57, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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.
>
I am happy for suggestions on better naming, but it would be better to
make a distinction somehow.
> 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.
>
Replaced with a bool alloc_id to decide whether it generates a new id
or not, and fixed.
> > +{
> > + 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?
>
This was a subset of what "release_resource_state" would have done,
since it erases a res_idx,
but on second thought, it's probably better to rename, so fixed as well.
Thanks for the review.
> > +{
> > + 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;
> > }
> > }
>
> [...]
>
