On Sat, Nov 9, 2024 at 1:38 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
>
> On Sat, 9 Nov 2024 at 22:30, Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Fri, Nov 8, 2024 at 11:43 PM Kumar Kartikeya Dwivedi
> > <memxor@xxxxxxxxx> wrote:
> > > struct bpf_retval_range {
> > > @@ -434,7 +431,7 @@ struct bpf_verifier_state {
> > > u32 insn_idx;
> > > u32 curframe;
> > >
> > > - struct bpf_active_lock active_lock;
> > > + int active_lock;
> >
> > What about this comment from v3:
> > > + bool active_lock;
> >
> > In the next patch it becomes 'int',
> > so let's make it 'int' right away and move it to bpf_func_state
> > next to:
> > int acquired_refs;
> > struct bpf_reference_state *refs;
> >
> > ?
>
> Ah, sorry, I somehow missed this part of the comment (twice). Mea culpa.
>
> >
> > wouldn't it be cleaner to keep the count of locks in bpf_func_state
> > next to refs
> >
> > acquire_lock_state() would increment it and release will dec it.
> >
> > check_resource_leak() will
> > instead of:
> > env->cur_state->active_lock
> > do:
> > cur_func(env)->active_lock
> >
> > so behavior is the same, but counting of locks is clean.
> >
> > Since in this patch it's kinda counting locks across all frames
> > which is a bit odd.
>
> It would work, but we'd need to copy it over to a new frame's
> bpf_func_state and copy it back on exit.
> None of that would matter currently as only one lock can be held, but
> it would become relevant later.
>
> It's the same situation with reference states. It is inherited from
> the parent frame for every new frame, and then possibly changed, and
> then copied back to parent frame.
Exactly, but since we're unifying held locks as references
would it make sense to treat them the same way everywhere?
active_lock is just a count of held locks in ref state.
For refs we do:
if (!exception_exit && state->in_callback_fn &&
state->refs[i].callback_ref != state->frameno)
so that
task = task_acquire().
bpf_loop(cb)
task_release(task)
doesn't trip.
Wouldn't we want the same for locks eventually ?
Just if (env->cur_state->active_lock) will change obviously.
We may even need two counters for regular and resilient locks ?
And in that case two counters in bpf_verifier_state looks even more odd.
While two counters in bpf_func_state feels more logical.
Just trying to anticipate the future changes...
