On Sat, 9 Nov 2024 at 23:02, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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 ?
>
This prompted me to dig into why (I) added that check. Turns out it
would no longer be necessary after Eduard fixed callback verification
some time ago.
It was a stop gap to prevent certain bad patterns since verifier
simulated a callback once. So preventing addition of references from
callback, and release of parent references prevented unsafe behavior
when callback actually ran N times. Now that's no longer necessary, so
I went ahead and dropped all of that cruft from the verifier, and the
selftests added back then still failed correctly.
Anyhow, all of it makes sense, so I went ahead and put active_locks as
part of bpf_func_state.
> 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...
