On Thu, 2024-11-21 at 19:12 +0100, Kumar Kartikeya Dwivedi wrote: > On Thu, 21 Nov 2024 at 19:09, Eduard Zingerman <eddyz87@xxxxxxxxx> 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@xxxxxxxxx> > > > --- > > > > 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@xxxxxxxxxxxxxx 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.
