On Thu, 21 Nov 2024 at 19:54, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > 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. > Discussed with Alexei (who discussed with you I presume) that we're doing this in bpf_verifier_state, will fix.
