Re: [PATCH RFC bpf-next v1 21/32] bpf: Allow locking bpf_spin_lock global variables

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>,
	Dave Marchevsky <davemarchevsky@fb.com>,
	bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Delyan Kratunov <delyank@fb.com>
Subject: Re: [PATCH RFC bpf-next v1 21/32] bpf: Allow locking bpf_spin_lock global variables
Date: Fri, 9 Sep 2022 11:32:40 -0700	[thread overview]
Message-ID: <CAEf4BzZL9GS0oAfkY1h4C9u1_XCzj-HTnKY9KHj+PX+h66TL3g@mail.gmail.com> (raw)
In-Reply-To: <CAADnVQLJP8YyYx5+mCBuSyenAfQDyXxDP8wfuDYCoZtO6kpunQ@mail.gmail.com>

On Fri, Sep 9, 2022 at 7:58 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Sep 9, 2022 at 7:51 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> >
> > On Fri, 9 Sept 2022 at 16:24, Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Fri, Sep 9, 2022 at 4:05 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > > >
> > > > On Fri, 9 Sept 2022 at 10:13, Dave Marchevsky <davemarchevsky@fb.com> wrote:
> > > > >
> > > > > On 9/4/22 4:41 PM, 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).
> > > > > >
> > > > >
> > > > > For libbpf-style "internal maps" - like .bss.private further in this series -
> > > > > all the SEC(".bss.private") vars are globbed together into one map_value. e.g.
> > > > >
> > > > >   struct bpf_spin_lock lock1 SEC(".bss.private");
> > > > >   struct bpf_spin_lock lock2 SEC(".bss.private");
> > > > >   ...
> > > > >   spin_lock(&lock1);
> > > > >   ...
> > > > >   spin_lock(&lock2);
> > > > >
> > > > > will result in same map but different offsets for the direct read (and different
> > > > > aux->map_off set in resolve_pseudo_ldimm64 for use in check_ld_imm). Seems like
> > > > > this patch would assign both same (active_spin_lock_ptr, active_spin_lock_id).
> > > > >
> > > >
> > > > That won't be a problem. Two spin locks in a map value or datasec are
> > > > already rejected on BPF_MAP_CREATE,
> > > > so there is no bug. See idx >= info_cnt check in
> > > > btf_find_struct_field, btf_find_datasec_var.
> > > >
> > > > I can include offset as the third part of the tuple. The problem then
> > > > is figuring out which lock protects which bpf_list_head. We need
> > > > another __guarded_by annotation and force users to use that to
> > > > eliminate the ambiguity. So for now I just put it in the commit log
> > > > and left it for the future.
> > >
> > > Let's not go that far yet.
> > > Extra annotations are just as confusing and non-obvious as
> > > putting locks in different sections.
> > > Let's keep one lock per map value limitation for now.
> > > libbpf side needs to allow many non-mappable sections though.
> > > Single bss.private name is too limiting.
> >
> > In that case,
> > Dave, since the libbpf patch is yours, would you be fine with
> > reworking it to support multiple private maps?
> > Maybe it can just ignore the .XXX part in .bss.private.XXX?
> > Also I think Andrii mentioned once that he wants to eventually merge
> > data and bss, so it might be a good idea to call it .data.private from
> > the start?
>
> I'd probably make all non-canonical names to be not-mmapable.
> The compiler generates special sections already.
> Thankfully the code doesn't use them, but it will sooner or later.
> So libbpf has to create hidden maps for them eventually.
> They shouldn't be messed up from user space, since it will screw up
> compiler generated code.
>
> Andrii, what's your take?

Ok, a bunch of things to unpack. We've also discussed a lot of this
with Dave few weeks ago, but I have also few questions.

First, I'd like to not keep extending ".bss" with any custom ".bss.*"
sections. This is why we have .data.* and .rodata.* and not .bss (bad,
meaningless, historic name).

But I'm totally fine dedicating some other prefix to non-mmapable data
sections that won't be exposed in skeleton and, well, not-mmapable.
What to name it depends on what we anticipate putting in them?

If it's just for spinlocks, then having something like SEC(".locks")
seems best to me. If it's for more stuff, like global kptrs, rbtrees
and whatnot, then we'd need a bit more generic name (.private, or
whatever, didn't think much on best name). We can also allow .locks.*
or .private.* (i.e., keep it uniform with .data and .rodata handling,
expect for mmapable aspect).

One benefit for having SEC(".locks") just for spin_locks is that we
can teach libbpf to create a multi-element ARRAY map, where each lock
variable is put into a separate element. From BPF verifier's
perspective, there will be a single BTF type describing spin lock, but
multiple "instances" of lock, one per each element. That seems a bit
magical and I think, generally speaking, it's best to start supporting
multiple lock declarations within single map element (and thus keep
track of their offset within map_value); but at least that's an
option.

Dave had some concerns about pinning such maps and whatnot, but for
starters we decided to not worry about pinning for now. Dave, please
bring up remaining issues, if you don't mind.

So to answer Alexei's specific option. I'm still not in favor of just
saying "anything that's not .data or .rodata is non-mmapable map". I'd
rather carve out naming prefixes with . (which are  reserved for
libbpf's own use) for these special purpose maps. I don't think that
limits anyone, right?