On Wed, Sep 7, 2022 at 6:15 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
>
> On Thu, 8 Sept 2022 at 03:09, Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > 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.
>
> map is only set when reg->type == PTR_TO_MAP_VALUE,
> otherwise btf = reg->btf for local kptrs (else branch). Then the map
> ptr is NULL.
> See patch 18 which already added support to local kptrs.
I see. That's what I was missing.
