Message-ID: <CAEf4BzYQFbngzELvyySd_f-otYOe74rH4ESNMDCEo5+PJw=umQ@mail.gmail.com>
Date:   Tue, 30 Mar 2021 11:00:43 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 3/3] selftests/bpf: allow compiling BPF objects
 without BTF

On Mon, Mar 29, 2021 at 11:56 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Sun, Mar 28, 2021 at 11:09:23PM -0700, Andrii Nakryiko wrote:
> >
> > BPF skeleton works just fine without BTF, if BPF programs don't use
> > global data. I have no way of knowing how BPF skeleton is used in the
> > wild, and specifically whether it is used without BTF and
> > .data/.rodata.
>
> No way of knowing?

Yes, of course I don't know all the ways that people use bpftool and
how they write applications. We can speculate about probability of
breaking someone's flow and how low chances are, but ultimately we are
guessing and hoping.

> The skel gen even for the most basic progs fails when there is no BTF in .o
>
> $ bpftool gen skeleton prog_compiled_without_dash_g.o
> libbpf: BTF is required, but is missing or corrupted.
>
> libbpf_needs_btf() check is preventing all but the most primitive progs.

Up until less than two years ago those were the only programs you
could write with libbpf. It's up to everyone's opinion to qualify them
as primitive or not. We even still have few selftests (which we should
convert, of course) that use bpf_map_def.

> Any prog with new style of map definition:
> struct {
>         __uint(type, BPF_MAP_TYPE_ARRAY);
>         __uint(max_entries, 1);
>         __type(key, __u32);
>         __type(value, __u64);
> } array SEC(".maps");
> would fail skel gen.
>
> bpftool is capable of skel gen for progs with old style maps only:
> struct bpf_map_def SEC("maps")
>

Yes, that's why my test is using a legacy-style map definition (which
for better or worse is still supported by libbpf). One can still write
full-fledged BPF applications without any BTF whatsoever.

> I think it's a safe bet that if folks didn't adopt new map definition
> they didn't use skeleton either.

I'm not going to argue, because I don't know. If I knew about BPF
skeleton but couldn't upgrade Clang, for instance, I'd still use BPF
skeleton to get nice access to maps/progs and get BPF object file
embedding in user-space without the hassle of distributing additional
.o.

>
> I think making skel gen reject such case is a good thing for the users,
> since it prevents them from creating maps that look like blob of bytes.
> It's good for admins too that more progs will get BTF described map key/value
> and systems are easier to debug.

I agree it's good, I added BTF-defined maps myself for that very reason.

>
> Ideally the kernel should reject loading progs and maps without BTF
> to guarantee introspection.
> Unfortunately the kernel backward compatibility prevents doing such
> drastic things.
> We might add a sysctl knob though.
>
> The bpftool can certainly add a message and reject .o-s without BTF.
> The chance of upsetting anyone is tiny.

Ok.

> Keep supporting old style 'bpf_map_def' is a maintenance burden.
> Sooner or later it needs to be removed not only from skel gen,
> but from libbpf as well.

I've already proposed to remove that in libbpf v1.0. See [0] for
discussion in the doc around that.

   [0] https://docs.google.com/document/d/1UyjTZuPFWiPFyKk1tV5an11_iaRuec6U-ZESZ54nNTY?disco=AAAALj68dg8

>
> > No one is asking for that, but they might be already using BTF-less
> > skeleton. So I'm fixing a bug in bpftool. In a way that doesn't cause
> > long term maintenance burden. And see above about my stance on tools'
> > assumptions.
>
> The patch and long term direction I'm arguing against is this one:
> https://patchwork.kernel.org/project/netdevbpf/patch/20210319205909.1748642-2-andrii@kernel.org/
> How is this a bug fix?
> From commit log:
> "If BPF object file is using global variables, but is compiled without BTF or
> ends up having only some of DATASEC types due to static linking"
>
> global vars without BTF were always rejected by bpftool

That's exactly what I consider a bug, because it wasn't intentional on my part.

> and should continue being rejected.
> I see no reason for adding such feature.
>
> > we both know this very well. But just as a fun exercise, I just
> > double-checked by compiling fentry demo from libbpf-bootstrap ([0]).
> > It works just fine without `-g` and BTF.
> >
> >   [0] https://github.com/libbpf/libbpf-bootstrap/blob/master/src/fentry.bpf.c
>
> yes. the skel gen will work for such demo prog, but the user should
> be making them introspectable.
>
> Try llvm-strip prog.o
> Old and new bpftool-s will simply crash, because there are no symbols.
> Should skel gen support such .o as well?

No, because libbpf doesn't support loading such BPF object files.
While my proposed patch was fixing the case in which libbpf would load
BPF object file.

> I don't think so. imo it's the same category of non-introspectable progs
> that shouldn't be allowed.
>

I understand. I just hope there was an opportunity to not always agree
100% with your opinions and have discussion without exaggerated
claims, like BPF skeleton not usable without BTF and others I tried to
address in this thread.

So, in summary, let's drop the patch.

> > Yeah, that's fine and we do require BTF for new features (where it
> > makes sense, of course, not just arbitrarily).
>
> I'm saying the kernel should enforce introspection.
> sysctl btf_enforced=1 might be the answer.

Powered by blists - more mailing lists