Message-ID: <20210329185558.mjoikgfdp53lq2it@ast-mbp>
Date:   Mon, 29 Mar 2021 11:55:58 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...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 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?
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.
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")

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

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.

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.
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.

> 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
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?
I don't think so. imo it's the same category of non-introspectable progs
that shouldn't be allowed.

> 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