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.
I have no preference either way, but if you think maintaining it as
part of func state is better I can make that change.
