On Thu, Sep 08, 2022 at 02:39:46AM +0200, Kumar Kartikeya Dwivedi 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.
> >
> > Also this patch is heavily influenced by Dave's patch with
> > a realization that max_entries==1 simplifies the logic.
>
> You mean this one?
> https://lore.kernel.org/bpf/20220830172759.4069786-12-davemarchevsky@xxxxxx
>
> > I think you gotta give him more credit.
> > Maybe as much as his SOB and authorship.
> >
>
> Don't mind sharing the credit where due, but for the record:
>
> 15/8: pushed my prototype:
> https://github.com/kkdwivedi/linux/commits/bpf-list-15-08-22
> 15/8: patch with roughly the same logic as above, comitted 24 days ago
> https://github.com/kkdwivedi/linux/commit/4a152df6a1f6e096616e02c9b4dd54c5d5c902a1
> 16/8: Our meeting, described the same idea to you.
> 17/8: Published notes,
> https://lore.kernel.org/bpf/CAP01T74U30+yeBHEgmgzTJ-XYxZ0zj71kqCDJtTH9YQNfTK+Xw@xxxxxxxxxxxxxx
> 19/8: Described the same thing in detail again in response to Dave's question:
> > This ergonomics idea doesn't solve the map-in-map issue, I'm still unsure
> > how to statically verify lock in that case. Have you had a chance to think
> > about it further?
> >
> at https://lore.kernel.org/bpf/CAP01T77PBfQ8QvgU-ezxGgUh8WmSYL3wsMT7yo4tGuZRW0qLnQ@xxxxxxxxxxxxxx
> 30/8: Dave sends patch with this idea:
> https://lore.kernel.org/bpf/20220830172759.4069786-11-davemarchevsky@xxxxxx
>
> What did I miss?
Just that I saw Dave's patch first. Yours 8-22 private branch I glanced over
and simply missed that patch. github UI is not for everyone.
As far as that notes thread it's hard to connect words to patches.
Re-reading it now I see what you mean.
Feel free to keep the authorship for this one.
Please answer btf question though.
