On Thu, 8 Sept 2022 at 02:27, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Sun, Sep 04, 2022 at 10:41:34PM +0200, 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@xxxxxxxxx>
> > ---
> > 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 2a9dcefca3b6..00c21ad6f61c 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -348,7 +348,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;
>
> {map, id=0} is indeed enough to distinguish different global locks and
> {map, id} for locks in map values,
> but what 'btf' is for?
> When is the case when reg->map_ptr is not set?
> locks in allocated objects?
> Feels too early to add that in this patch.
>
It makes active_spin_lock check simpler, just checking
active_spin_lock_ptr that to be non-NULL indicates lock is held. Don't
have to always check both ptr and id, only need to compare both when
verifying that lock is in the same allocation as reg.
