On Wed, Sep 7, 2022 at 6:01 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
>
> 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.
Not following. There is always non-null reg->map_ptr when
we come down this path.
At least in the current state of the verifier.
So it never assigns that btf afacs.
