Re: [PATCH bpf-next v1 3/7] bpf: Consolidate RCU and preempt locks in bpf_func_state

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2024-11-21 at 19:12 +0100, Kumar Kartikeya Dwivedi wrote:
> On Thu, 21 Nov 2024 at 19:09, Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Wed, 2024-11-20 at 16:53 -0800, Kumar Kartikeya Dwivedi wrote:
> > > To ensure consistency in resource handling, move RCU and preemption
> > > state counters to bpf_func_state, and convert all users to access them
> > > through cur_func(env).
> > > 
> > > For the sake of consistency, also compare active_locks in ressafe as a
> > > quick way to eliminate iteration and entry matching if the number of
> > > locks are not the same.
> > > 
> > > OTOH, the comparison of active_preempt_locks and active_rcu_lock is
> > > needed for correctness, as state exploration cannot be avoided if these
> > > counters do not match, and not comparing them will lead to problems
> > > since they lack an actual entry in the acquired_res array.
> > > 
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > ---
> > 
> > This change is a bit confusing to me.
> > The following is done currently:
> > - in setup_func_entry() called from check_func_call():
> >   copy_resource_state(callee, caller);
> > - in prepare_func_exit():
> >   copy_resource_state(caller, callee);
> > 
> > So it seems that it is logical to track resources in the
> > bpf_verifier_state and avoid copying.
> > There is probably something I don't understand.
> > 
> 
> This is what we were doing all along, and you're right, it is sort of
> a global entity.

Right, but since this patch-set does a refactoring,
might be a good time to change.

> But we've moved active_locks to bpf_func_state, where references reside, while
> RCU and preempt lock state stays in verifier state. Either everything
> should be in
> cur_func, or in bpf_verifier_state. I am fine with either of them,
> because it would
> materially does not matter too much.
> 
> Alexei's preference has been stashing this in bpf_func_state instead in [0].
> Let me know what you think.
> 
>   [0] https://lore.kernel.org/bpf/CAADnVQKxgE7=WhjNckvMDTZ5GZujPuT3Dqd+sY=pW8CWoaF9FA@mail.gmail.com

As far as I understand check_func_call(), function calls to static
functions are allowed while holding each kind of resources currently
tracked. So it seems odd to track it as a part of function state.
The way I understand Alexei in the thread [0] the idea is more
to track all counters in one place.

Let's wait what Alexei has to say.