Re: [PATCH bpf-next v4 14/24] bpf: Allow locking bpf_spin_lock global variables

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Fri, Nov 04, 2022 at 08:24:22AM IST, Dave Marchevsky wrote:
> On 11/3/22 3:10 PM, Kumar Kartikeya Dwivedi wrote:
> > Global variables reside in maps accessible using direct_value_addr
> > callbacks, so giving each load instruction's rewrite a unique reg->id
> > disallows us from holding locks which are global.
> >
> > This is not great, so refactor the active_spin_lock into two separate
> > fields, active_spin_lock_ptr and active_spin_lock_id, which is generic
> > enough to allow it for global variables, map lookups, and local kptr
> > registers at the same time.
> >
> > Held vs non-held is indicated by active_spin_lock_ptr, which stores the
> > reg->map_ptr or reg->btf pointer of the register used for locking spin
> > lock. But the active_spin_lock_id also needs to be compared to ensure
> > whether bpf_spin_unlock is for the same register.
> >
> > Next, pseudo load instructions are not given a unique reg->id, as they
> > are doing lookup for the same map value (max_entries is never greater
> > than 1).
> >
> > Essentially, we consider that the tuple of (active_spin_lock_ptr,
> > active_spin_lock_id) will always be unique for any kind of argument to
> > bpf_spin_{lock,unlock}.
> >
> > Note that this can be extended in the future to also remember offset
> > used for locking, so that we can introduce multiple bpf_spin_lock fields
> > in the same allocation.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/bpf_verifier.h |  3 ++-
> >  kernel/bpf/verifier.c        | 39 +++++++++++++++++++++++++-----------
> >  2 files changed, 29 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index 1a32baa78ce2..bb71c59f21f6 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -323,7 +323,8 @@ struct bpf_verifier_state {
> >  	u32 branches;
> >  	u32 insn_idx;
> >  	u32 curframe;
> > -	u32 active_spin_lock;
> > +	void *active_spin_lock_ptr;
> > +	u32 active_spin_lock_id;
> >  	bool speculative;
> Back in first RFC of this series we talked about turning this "spin lock
> identity" concept into a proper struct [0]. But to save you the click:
>
> Dave:
> """
> It would be good to make this "(lock_ptr, lock_id) is identifier for lock"
> concept more concrete by grouping these fields in a struct w/ type enum + union,
> or something similar. Will make it more obvious that they should be used / set
> together.
>
> But if you'd prefer to keep it as two fields, active_spin_lock_ptr is a
> confusing name. In the future with no context as to what that field is, I'd
> assume that it holds a pointer to a spin_lock instead of a "spin lock identity
> pointer".
> """
>
> Kumar:
> """
> That's a good point.
>
> I'm thinking
> struct active_lock {
>   void *id_ptr;
>   u32 offset;
>   u32 reg_id;
> };
> How does that look?
> """
>
> I didn't get back to you, but I think that looks reasonable, and "this can be
> extended in the future to also remember offset used for locking" in this
> patch summary supports the desire to group up those fields. (I agree that
> offset isn't necessary for now, though).
>

I will make this change in v5.

However, do you have any suggestions on what we can call the id_ptr thing? In
patch 22 in the big comment above check_reg_allocation_locked I call it lock
class, but I'm not sure whether it helps or is more confusing for people.

In active_spin_lock_ptr, 'ptr' alone is confusing as you've pointed out before.