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?
