diff mbox series

[bpf-next,3/3] selftests/bpf: allow compiling BPF objects without BTF

Message ID 20210319205909.1748642-4-andrii@kernel.org
State New
Headers show
Series Handle no-BTF object files better | expand

Commit Message

Andrii Nakryiko March 19, 2021, 8:59 p.m. UTC
Add ability to skip BTF generation for some BPF object files. This is done
through using a convention of .nobtf.c file name suffix.

Also add third statically linked file to static_linked selftest. This file has
no BTF, causing resulting object file to have only some of DATASEC BTF types.
It also is using (from BPF code) global variables. This tests both libbpf's
static linking logic and bpftool's skeleton generation logic.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/Makefile          | 21 +++++++----
 .../selftests/bpf/prog_tests/static_linked.c  |  6 +++-
 .../bpf/progs/test_static_linked3.nobtf.c     | 36 +++++++++++++++++++
 3 files changed, 56 insertions(+), 7 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_static_linked3.nobtf.c

Comments

Alexei Starovoitov March 20, 2021, 2:21 a.m. UTC | #1
On Fri, Mar 19, 2021 at 01:59:09PM -0700, Andrii Nakryiko wrote:
> Add ability to skip BTF generation for some BPF object files. This is done

> through using a convention of .nobtf.c file name suffix.

> 

> Also add third statically linked file to static_linked selftest. This file has

> no BTF, causing resulting object file to have only some of DATASEC BTF types.

> It also is using (from BPF code) global variables. This tests both libbpf's

> static linking logic and bpftool's skeleton generation logic.


I don't like the long term direction of patch 1 and 3.
BTF is mandatory for the most bpf kernel features added in the last couple years.
Making user space do quirks for object files without BTF is not something
we should support or maintain. If there is no BTF the linker and skeleton
generation shouldn't crash, of course, but they should reject such object.
Andrii Nakryiko March 20, 2021, 5 p.m. UTC | #2
On Fri, Mar 19, 2021 at 7:22 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>

> On Fri, Mar 19, 2021 at 01:59:09PM -0700, Andrii Nakryiko wrote:

> > Add ability to skip BTF generation for some BPF object files. This is done

> > through using a convention of .nobtf.c file name suffix.

> >

> > Also add third statically linked file to static_linked selftest. This file has

> > no BTF, causing resulting object file to have only some of DATASEC BTF types.

> > It also is using (from BPF code) global variables. This tests both libbpf's

> > static linking logic and bpftool's skeleton generation logic.

>

> I don't like the long term direction of patch 1 and 3.

> BTF is mandatory for the most bpf kernel features added in the last couple years.

> Making user space do quirks for object files without BTF is not something

> we should support or maintain. If there is no BTF the linker and skeleton

> generation shouldn't crash, of course, but they should reject such object.


I don't think tools need to enforce any policies like that. They are
tools and should be unassuming about the way they are going to be used
to the extent possible. If BTF is useful it will happen naturally that
everyone will use it (and it's mostly the case already). But I don't
think crippling tools for the sake of enforcing BTF is the right
direction.

And it's not much of a quirk what bpftool is doing. If you prefer, I
can generate `void *` field for memory-mapped pointer. Patch #3 just
allows me to validate that there are no crashes.
Alexei Starovoitov March 22, 2021, 1:07 a.m. UTC | #3
On Sat, Mar 20, 2021 at 10:00:57AM -0700, Andrii Nakryiko wrote:
> On Fri, Mar 19, 2021 at 7:22 PM Alexei Starovoitov

> <alexei.starovoitov@gmail.com> wrote:

> >

> > On Fri, Mar 19, 2021 at 01:59:09PM -0700, Andrii Nakryiko wrote:

> > > Add ability to skip BTF generation for some BPF object files. This is done

> > > through using a convention of .nobtf.c file name suffix.

> > >

> > > Also add third statically linked file to static_linked selftest. This file has

> > > no BTF, causing resulting object file to have only some of DATASEC BTF types.

> > > It also is using (from BPF code) global variables. This tests both libbpf's

> > > static linking logic and bpftool's skeleton generation logic.

> >

> > I don't like the long term direction of patch 1 and 3.

> > BTF is mandatory for the most bpf kernel features added in the last couple years.

> > Making user space do quirks for object files without BTF is not something

> > we should support or maintain. If there is no BTF the linker and skeleton

> > generation shouldn't crash, of course, but they should reject such object.

> 

> I don't think tools need to enforce any policies like that. They are

> tools and should be unassuming about the way they are going to be used

> to the extent possible.


Right and bpftool/skeleton was used with BTF since day one.
Without BTF the skeleton core ideas are lost. The skeleton api
gives no benefit. So what's the point of adding support for skeleton without BTF?
Is there a user that would benefit? If so, what will they gain from
such BTF-less skeleton?
Andrii Nakryiko March 22, 2021, 4:56 p.m. UTC | #4
On Sun, Mar 21, 2021 at 6:07 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>

> On Sat, Mar 20, 2021 at 10:00:57AM -0700, Andrii Nakryiko wrote:

> > On Fri, Mar 19, 2021 at 7:22 PM Alexei Starovoitov

> > <alexei.starovoitov@gmail.com> wrote:

> > >

> > > On Fri, Mar 19, 2021 at 01:59:09PM -0700, Andrii Nakryiko wrote:

> > > > Add ability to skip BTF generation for some BPF object files. This is done

> > > > through using a convention of .nobtf.c file name suffix.

> > > >

> > > > Also add third statically linked file to static_linked selftest. This file has

> > > > no BTF, causing resulting object file to have only some of DATASEC BTF types.

> > > > It also is using (from BPF code) global variables. This tests both libbpf's

> > > > static linking logic and bpftool's skeleton generation logic.

> > >

> > > I don't like the long term direction of patch 1 and 3.

> > > BTF is mandatory for the most bpf kernel features added in the last couple years.

> > > Making user space do quirks for object files without BTF is not something

> > > we should support or maintain. If there is no BTF the linker and skeleton

> > > generation shouldn't crash, of course, but they should reject such object.

> >

> > I don't think tools need to enforce any policies like that. They are

> > tools and should be unassuming about the way they are going to be used

> > to the extent possible.

>

> Right and bpftool/skeleton was used with BTF since day one.

> Without BTF the skeleton core ideas are lost. The skeleton api

> gives no benefit. So what's the point of adding support for skeleton without BTF?

> Is there a user that would benefit? If so, what will they gain from

> such BTF-less skeleton?


The only part of skeleton API that's not available is convenient
user-space access to global variables. If you don't use global
variables you don't use BTF at all with skeleton. So all features but
one work without BTF just fine: compile-time maps and progs (and
links) references, embedding object file in .skel.h, and even
automatic memory-mapping of .data/.rodata/.bss (just unknown struct
layout).

Compile-time maps and progs and separately object file embedding in C
header are useful in their own rights, even individually. There is no
single "core idea" of the BPF skeleton in my mind. What is it for you?

So given none of the fixes are horrible hacks and won't incur
additional maintenance costs, what's the problem with accepting them?
Alexei Starovoitov March 22, 2021, 5:54 p.m. UTC | #5
On Mon, Mar 22, 2021 at 09:56:19AM -0700, Andrii Nakryiko wrote:
> On Sun, Mar 21, 2021 at 6:07 PM Alexei Starovoitov

> <alexei.starovoitov@gmail.com> wrote:

> >

> > On Sat, Mar 20, 2021 at 10:00:57AM -0700, Andrii Nakryiko wrote:

> > > On Fri, Mar 19, 2021 at 7:22 PM Alexei Starovoitov

> > > <alexei.starovoitov@gmail.com> wrote:

> > > >

> > > > On Fri, Mar 19, 2021 at 01:59:09PM -0700, Andrii Nakryiko wrote:

> > > > > Add ability to skip BTF generation for some BPF object files. This is done

> > > > > through using a convention of .nobtf.c file name suffix.

> > > > >

> > > > > Also add third statically linked file to static_linked selftest. This file has

> > > > > no BTF, causing resulting object file to have only some of DATASEC BTF types.

> > > > > It also is using (from BPF code) global variables. This tests both libbpf's

> > > > > static linking logic and bpftool's skeleton generation logic.

> > > >

> > > > I don't like the long term direction of patch 1 and 3.

> > > > BTF is mandatory for the most bpf kernel features added in the last couple years.

> > > > Making user space do quirks for object files without BTF is not something

> > > > we should support or maintain. If there is no BTF the linker and skeleton

> > > > generation shouldn't crash, of course, but they should reject such object.

> > >

> > > I don't think tools need to enforce any policies like that. They are

> > > tools and should be unassuming about the way they are going to be used

> > > to the extent possible.

> >

> > Right and bpftool/skeleton was used with BTF since day one.

> > Without BTF the skeleton core ideas are lost. The skeleton api

> > gives no benefit. So what's the point of adding support for skeleton without BTF?

> > Is there a user that would benefit? If so, what will they gain from

> > such BTF-less skeleton?

> 

> The only part of skeleton API that's not available is convenient

> user-space access to global variables. If you don't use global

> variables you don't use BTF at all with skeleton. So all features but

> one work without BTF just fine: compile-time maps and progs (and

> links) references, embedding object file in .skel.h, and even

> automatic memory-mapping of .data/.rodata/.bss (just unknown struct

> layout).

> 

> Compile-time maps and progs and separately object file embedding in C

> header are useful in their own rights, even individually. There is no

> single "core idea" of the BPF skeleton in my mind. What is it for you?

> 

> So given none of the fixes are horrible hacks and won't incur

> additional maintenance costs, what's the problem with accepting them?


Because they double the maintenance cost now and double the support forever.
We never needed to worry about skeleton without BTF and now it would be
a thing ? So all tests realistically need to be doubled: with and without BTF.
Even more so for static linking. If one .o has BTF and another doesn't
what linker suppose to do? Keep it, but the linked BTF will sort of cover
both .o-s, but line info in some funcs will be missing.
All these weird combinations would need to be tested.
The sensible thing to do would be to reject skel generation without BTF
and reject linking without BTF. The user most likely forgot -g during
compilation of bpf prog. The bpftool should give the helpful message
in such case. Whether it's generating skel or linking. Silently proceeding
and generating half-featured skeleton is not what user wanted.
Andrii Nakryiko March 26, 2021, 4:44 p.m. UTC | #6
On Mon, Mar 22, 2021 at 10:54 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>

> On Mon, Mar 22, 2021 at 09:56:19AM -0700, Andrii Nakryiko wrote:

> > On Sun, Mar 21, 2021 at 6:07 PM Alexei Starovoitov

> > <alexei.starovoitov@gmail.com> wrote:

> > >

> > > On Sat, Mar 20, 2021 at 10:00:57AM -0700, Andrii Nakryiko wrote:

> > > > On Fri, Mar 19, 2021 at 7:22 PM Alexei Starovoitov

> > > > <alexei.starovoitov@gmail.com> wrote:

> > > > >

> > > > > On Fri, Mar 19, 2021 at 01:59:09PM -0700, Andrii Nakryiko wrote:

> > > > > > Add ability to skip BTF generation for some BPF object files. This is done

> > > > > > through using a convention of .nobtf.c file name suffix.

> > > > > >

> > > > > > Also add third statically linked file to static_linked selftest. This file has

> > > > > > no BTF, causing resulting object file to have only some of DATASEC BTF types.

> > > > > > It also is using (from BPF code) global variables. This tests both libbpf's

> > > > > > static linking logic and bpftool's skeleton generation logic.

> > > > >

> > > > > I don't like the long term direction of patch 1 and 3.

> > > > > BTF is mandatory for the most bpf kernel features added in the last couple years.

> > > > > Making user space do quirks for object files without BTF is not something

> > > > > we should support or maintain. If there is no BTF the linker and skeleton

> > > > > generation shouldn't crash, of course, but they should reject such object.

> > > >

> > > > I don't think tools need to enforce any policies like that. They are

> > > > tools and should be unassuming about the way they are going to be used

> > > > to the extent possible.

> > >

> > > Right and bpftool/skeleton was used with BTF since day one.

> > > Without BTF the skeleton core ideas are lost. The skeleton api

> > > gives no benefit. So what's the point of adding support for skeleton without BTF?

> > > Is there a user that would benefit? If so, what will they gain from

> > > such BTF-less skeleton?

> >

> > The only part of skeleton API that's not available is convenient

> > user-space access to global variables. If you don't use global

> > variables you don't use BTF at all with skeleton. So all features but

> > one work without BTF just fine: compile-time maps and progs (and

> > links) references, embedding object file in .skel.h, and even

> > automatic memory-mapping of .data/.rodata/.bss (just unknown struct

> > layout).

> >

> > Compile-time maps and progs and separately object file embedding in C

> > header are useful in their own rights, even individually. There is no

> > single "core idea" of the BPF skeleton in my mind. What is it for you?

> >

> > So given none of the fixes are horrible hacks and won't incur

> > additional maintenance costs, what's the problem with accepting them?

>

> Because they double the maintenance cost now and double the support forever.

> We never needed to worry about skeleton without BTF and now it would be

> a thing ? So all tests realistically need to be doubled: with and without BTF.


2x? Realistically?.. No, I wouldn't say so. :) Extra test or a few
might be warranted, but doubling the amount of testing is a huge
exaggeration.

> Even more so for static linking. If one .o has BTF and another doesn't

> what linker suppose to do? Keep it, but the linked BTF will sort of cover

> both .o-s, but line info in some funcs will be missing.

> All these weird combinations would need to be tested.


BPF static linker already supports that mode, btw. And yes, it
shouldn't crash the kernel. And you don't need a skeleton or static
linker to do that to the kernel, so I don't know how that is a new
mode of operation.

> The sensible thing to do would be to reject skel generation without BTF

> and reject linking without BTF. The user most likely forgot -g during

> compilation of bpf prog. The bpftool should give the helpful message

> in such case. Whether it's generating skel or linking. Silently proceeding

> and generating half-featured skeleton is not what user wanted.


Sure, a warning message makes sense. Outright disabling this - not so
much. I still can't get why I can't get BPF skeleton and static
linking without BTF, if I really want to. Both are useful without BTF.

So I don't know, it's the third different argument I'm addressing
without any conclusion on the previous two. It, sadly, feels rather
like fighting subjective personal preferences, rather than a
discussion.
Alexei Starovoitov March 29, 2021, 1:16 a.m. UTC | #7
On Fri, Mar 26, 2021 at 9:44 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> > Because they double the maintenance cost now and double the support forever.

> > We never needed to worry about skeleton without BTF and now it would be

> > a thing ? So all tests realistically need to be doubled: with and without BTF.

>

> 2x? Realistically?.. No, I wouldn't say so. :) Extra test or a few

> might be warranted, but doubling the amount of testing is a huge

> exaggeration.


It's not only doubling the test coverage, but it would double the development
effort to keep everything w/BTF and w/o BTF functional.
So 2x is far from exaggeration.

> > Even more so for static linking. If one .o has BTF and another doesn't

> > what linker suppose to do? Keep it, but the linked BTF will sort of cover

> > both .o-s, but line info in some funcs will be missing.

> > All these weird combinations would need to be tested.

>

> BPF static linker already supports that mode, btw.


Are you considering one-liner commit 78b226d48106 ("libbpf: Skip BTF
fixup if object file has no BTF")
as support for static linking w/o BTF?
Jiri's other email pointed out another place in libbpf that breaks w/o BTF.
The only thing the commit 78b226d48106 achieved is it closed
the one case of static linker crashing w/o BTF.
Does linker do anything sensible when a mix of .o with and without BTF
is given? No.
It happens not to crash w/o BTF. That's about it.

> And yes, it

> shouldn't crash the kernel. And you don't need a skeleton or static

> linker to do that to the kernel, so I don't know how that is a new

> mode of operation.

>

> > The sensible thing to do would be to reject skel generation without BTF

> > and reject linking without BTF. The user most likely forgot -g during

> > compilation of bpf prog. The bpftool should give the helpful message

> > in such case. Whether it's generating skel or linking. Silently proceeding

> > and generating half-featured skeleton is not what user wanted.

>

> Sure, a warning message makes sense. Outright disabling this - not so

> much. I still can't get why I can't get BPF skeleton and static

> linking without BTF, if I really want to. Both are useful without BTF.


What are the cases that would benefit?
So far skeleton was used by tracing progs only. Those that need CO-RE.
Which means they must have BTF.
Networking progs didn't need CO-RE and no one bothered adopting
skeleton because of that.
Are you implying that a BTF-less skeleton will be useful for
networking progs? What for?
So far I see only downsides from increasing the amount of work needed
to support skel and static linking w/o BTF. The extra 100% or just 1%
would be equally taxing, since it's extra work for the foreseeable future and
compounded 1% will add up.

Looking at it from another angle... the skeleton generation existed
for more than
a year now and it implicitly required BTF. That requirement was not
written down.
Now you're proposing to support skeleton gen w/o BTF when not a single user
requested it and not providing any advantages of such support while
ignoring the long term maintenance of such effort.

Another angle... I did git grep in selftests that use skeleton.
Only a handful of tests use it as *skel*__open_and_load() w/o global data
(which would still work w/o BTF)
and only because we forcefully converted them when skel was introduced.
bcc/libbpf-tools/* need skel with BTF.
I couldn't find a _single_ case where people use
skeleton and don't need BTF driven parts of it.

> So I don't know, it's the third different argument I'm addressing

without any conclusion on the previous two.

So far you haven't addressed the first question:
Who is asking for a BTF-less skeleton? What for? What features are requested?
I've seen none of such requests.

BTF is not a debug info of the BPF program.
If it was then I would agree that compiling with and without -g shouldn't
make a difference.
But BTF is not a debug-info. It's type and much more description of the program
that is mandatory for the verification of the program.
btf_id-based attach, CO-RE, trampoline, calling kernel funcs, etc all
require BTF.
Not because it's convenient to use BTF, but because assembly doesn't
have enough information.
There is no C, C++, Rust equivalent of BTF. There is none in the user
space world.
Traditional languages translate a language into assembly code and cpus
execute it.
Static analyzers use high level languages to understand the intent.
BPF workflow translates a language into assembly and BTF, so that the verifier
can see the intent. It could happen that BPF will gain something else beyond BTF
and it will become a mandatory part of the workflow. Just like BTF is today.
At that point all new features will be supported with that new
"annotation" only,
not because of "subjective personal preferences", but because that is
a fundamental
program description.
Andrii Nakryiko March 29, 2021, 6:09 a.m. UTC | #8
On Sun, Mar 28, 2021 at 6:16 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>

> On Fri, Mar 26, 2021 at 9:44 AM Andrii Nakryiko

> <andrii.nakryiko@gmail.com> wrote:

> > > Because they double the maintenance cost now and double the support forever.

> > > We never needed to worry about skeleton without BTF and now it would be

> > > a thing ? So all tests realistically need to be doubled: with and without BTF.

> >

> > 2x? Realistically?.. No, I wouldn't say so. :) Extra test or a few

> > might be warranted, but doubling the amount of testing is a huge

> > exaggeration.

>

> It's not only doubling the test coverage, but it would double the development

> effort to keep everything w/BTF and w/o BTF functional.

> So 2x is far from exaggeration.


We must be talking about different things, because I don't understand
where you get all this doubling of development effort from.

>

> > > Even more so for static linking. If one .o has BTF and another doesn't

> > > what linker suppose to do? Keep it, but the linked BTF will sort of cover

> > > both .o-s, but line info in some funcs will be missing.

> > > All these weird combinations would need to be tested.

> >

> > BPF static linker already supports that mode, btw.

>

> Are you considering one-liner commit 78b226d48106 ("libbpf: Skip BTF

> fixup if object file has no BTF")

> as support for static linking w/o BTF?


No, of course not. I'm talking about 8fd27bf69b86 ("libbpf: Add BPF
static linker BTF and BTF.ext support"), which does sensible thing
when some of input object files miss BTF. It still generates valid BTF
with as much data as it is possible to collect from the remaining
object files' BTFs. All the DATASECs will still have variable info and
correct offsets for all the variables that have BTF associated with
them.

78b226d48106 ("libbpf: Skip BTF fixup if object file has no BTF") was
a bug fix for one condition I missed in the original commit. I feel
bad about the inconveniences it caused to you and Jiri, but bugs do
slip up. Which is why I added selftests in this patch, to catch that
next time.


> Jiri's other email pointed out another place in libbpf that breaks w/o BTF.


Jiri's last issue is with add_dummy_ksym_var(), added recently in
kernel function calls support patch set. add_dummy_ksym_var()
shouldn't be called if bpf_object is missing BTF. Previously
find_extern_btf_id() would return -ESRCH in such a case, but
add_dummy_ksym_var() is now called before it.

> The only thing the commit 78b226d48106 achieved is it closed

> the one case of static linker crashing w/o BTF.

> Does linker do anything sensible when a mix of .o with and without BTF

> is given? No.

> It happens not to crash w/o BTF. That's about it.


No, see above.

>

> > And yes, it

> > shouldn't crash the kernel. And you don't need a skeleton or static

> > linker to do that to the kernel, so I don't know how that is a new

> > mode of operation.

> >

> > > The sensible thing to do would be to reject skel generation without BTF

> > > and reject linking without BTF. The user most likely forgot -g during

> > > compilation of bpf prog. The bpftool should give the helpful message

> > > in such case. Whether it's generating skel or linking. Silently proceeding

> > > and generating half-featured skeleton is not what user wanted.

> >

> > Sure, a warning message makes sense. Outright disabling this - not so

> > much. I still can't get why I can't get BPF skeleton and static

> > linking without BTF, if I really want to. Both are useful without BTF.

>

> What are the cases that would benefit?

> So far skeleton was used by tracing progs only. Those that need CO-RE.

> Which means they must have BTF.

> Networking progs didn't need CO-RE and no one bothered adopting

> skeleton because of that.

> Are you implying that a BTF-less skeleton will be useful for

> networking progs? What for?


Yes, I'm outright claiming that it would be useful even without BTF.
skel->progs.my_prog and skel->maps.my_map is extremely useful and cuts
down on boilerplate and accidental prog/map name typos, saving
development time. And it was used for that reason even in
selftests/bpf, just to make tests shorter and nicer. Auto mmap-ed
.data/.rodata, even without BTF, are still usable, if you collect all
your variables into a single struct. Which was the typical case before
the BPF skeleton came about.

So, not having to manually mmap() .rodata/.data/.bss is beneficial.
And it doesn't come at a lot of cost. But again, I'm not advocating
for such an approach, and I'm not saying we should encourage it. But
as I already said, I like tools that don't make unnecessary
assumptions, especially if that doesn't come at huge cost, which I
still think is the case here.

As for why networking folks haven't adopted it. I'd ask networking
folks, but I assume inertia has its place there as well, which is to
be expected and is not bad per se. Changes take time. If they already
have working applications, why bother changing anything?

> So far I see only downsides from increasing the amount of work needed

> to support skel and static linking w/o BTF. The extra 100% or just 1%

> would be equally taxing, since it's extra work for the foreseeable future and

> compounded 1% will add up.

>

> Looking at it from another angle... the skeleton generation existed

> for more than

> a year now and it implicitly required BTF. That requirement was not

> written down.

> Now you're proposing to support skeleton gen w/o BTF when not a single user

> requested it and not providing any advantages of such support while

> ignoring the long term maintenance of such effort.


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. But artificially failing BPF skeleton generation now
for such a case would be an obvious regression.

I think those maintenance costs are exaggerated, we are probably not
going to agree on this. I fixed the bug in BPF skeleton making
unnecessary assumptions of BTF DATASEC presence, where it's not
strictly necessary for correctness. I don't see it as an ugly hack or
maintenance burden. It shouldn't need constant attention and shouldn't
break.

>

> Another angle... I did git grep in selftests that use skeleton.

> Only a handful of tests use it as *skel*__open_and_load() w/o global data

> (which would still work w/o BTF)

> and only because we forcefully converted them when skel was introduced.

> bcc/libbpf-tools/* need skel with BTF.

> I couldn't find a _single_ case where people use

> skeleton and don't need BTF driven parts of it.


And I've found a few at Facebook that don't need any BTF and use
open_and_load() and skel->maps and skel->progs accessor. But I don't
know what either of those prove?.. libbpf-tools obviously are using
BTF, because I've set up everything so that BTF is always generated
and I've suggested use of global variables for each converted tool.
But again, how is that an argument for anything. We can't know
everything that people are doing.

I'll say that again. I hate when tools assume how I'm going to use
them. They should do what they are meant to do and don't impose
unnecessary restrictions.

>

> > So I don't know, it's the third different argument I'm addressing

> without any conclusion on the previous two.

>

> So far you haven't addressed the first question:

> Who is asking for a BTF-less skeleton? What for? What features are requested?

> I've seen none of such requests.


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.

BPF skeleton wasn't added specifically to access global variables.
skel->maps and skel->progs were enough motivation for me to add BPF
skeleton. And still is enough.


>

> BTF is not a debug info of the BPF program.

> If it was then I would agree that compiling with and without -g shouldn't

> make a difference.


It effectively doesn't in a lot of cases (all the "classic" BPF
programs that used kprobes, tracepoints, perfbuf, maps, etc). But I'm
not arguing for not using BTF at all. I've myself contributed a lot of
time and effort to make BTF almost universal in BPF ecosystem. If I
didn't believe in BTF, I wouldn't add BTF-defined maps, just as a one
example of reliance on BTF.

All I'm asking is to let me fix a bug in BPF skeleton generation and
have a test validating that it is fixed.

> But BTF is not a debug-info. It's type and much more description of the program

> that is mandatory for the verification of the program.

> btf_id-based attach, CO-RE, trampoline, calling kernel funcs, etc all

> require BTF.


If fentry/fexit (whether it is btf_id-based or trampoline in your
classification), then BTF for the BPF program itself is not required
for it to work at all. Only kernel BTF is a requirement for those. But
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


> Not because it's convenient to use BTF, but because assembly doesn't

> have enough information.

> There is no C, C++, Rust equivalent of BTF. There is none in the user

> space world.


Right, because of CO-RE relocations.

> Traditional languages translate a language into assembly code and cpus

> execute it.

> Static analyzers use high level languages to understand the intent.

> BPF workflow translates a language into assembly and BTF, so that the verifier

> can see the intent. It could happen that BPF will gain something else beyond BTF

> and it will become a mandatory part of the workflow. Just like BTF is today.


Yeah, that's fine and we do require BTF for new features (where it
makes sense, of course, not just arbitrarily). Both in kernel and in
libbpf. But kernel doesn't artificially impose BTF restrictions
either. See above about fentry/fexit, they work just fine without BTF.
So do a lot of other features. Same for libbpf, see libbpf_needs_btf()
and kernel_needs_btf().


> At that point all new features will be supported with that new

> "annotation" only,

> not because of "subjective personal preferences", but because that is

> a fundamental

> program description.


Map relocations, BPF-to-BPF calls, global variables, even multiple BPF
programs per-section are all pretty fundamental to BPF programming
with libbpf and they work just fine without BTF. But I agree, BTF is
great and should be leveraged where it provides extra value.
Alexei Starovoitov March 29, 2021, 6:55 p.m. UTC | #9
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.
Andrii Nakryiko March 30, 2021, 6 p.m. UTC | #10
On Mon, Mar 29, 2021 at 11:56 AM Alexei Starovoitov
<alexei.starovoitov@gmail.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.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 6448c626498f..0a481a75a416 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -270,7 +270,7 @@  IS_LITTLE_ENDIAN = $(shell $(CC) -dM -E - </dev/null | \
 MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)
 
 CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
-BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) 			\
+BPF_CFLAGS = -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) 			\
 	     -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR)			\
 	     -I$(abspath $(OUTPUT)/../usr/include)
 
@@ -282,30 +282,39 @@  $(OUTPUT)/test_xdp_noinline.o: BPF_CFLAGS += -fno-inline
 
 $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
 
-# Build BPF object using Clang
+# Build BPF object using Clang.
+# Source files with .nobtf.c suffix are built without BTF
 # $1 - input .c file
 # $2 - output .o file
 # $3 - CFLAGS
 define CLANG_BPF_BUILD_RULE
 	$(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
-	$(Q)$(CLANG) $3 -O2 -target bpf -c $1 -o $2 -mcpu=v3
+	$(Q)$(CLANG) $3 -O2 -target bpf -mcpu=v3			\
+		     $(if $(filter %.nobtf.c,$1),,-g)			\
+		     -c $1 -o $2
 endef
 # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32
 define CLANG_NOALU32_BPF_BUILD_RULE
 	$(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2)
-	$(Q)$(CLANG) $3 -O2 -target bpf -c $1 -o $2 -mcpu=v2
+	$(Q)$(CLANG) $3 -O2 -target bpf -mcpu=v2			\
+		     $(if $(filter %.nobtf.c,$1),,-g)			\
+		     -c $1 -o $2
 endef
 # Build BPF object using GCC
 define GCC_BPF_BUILD_RULE
 	$(call msg,GCC-BPF,$(TRUNNER_BINARY),$2)
-	$(Q)$(BPF_GCC) $3 -O2 -c $1 -o $2
+	$(Q)$(BPF_GCC) $3 -O2 						\
+		       $(if $(filter %.nobtf.c,$1),,-g)			\
+		       -c $1 -o $2
 endef
 
 SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
 
 LINKED_SKELS := test_static_linked.skel.h
 
-test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o
+test_static_linked.skel.h-deps := test_static_linked1.o \
+				  test_static_linked2.o \
+				  test_static_linked3.nobtf.o
 
 # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on
 # $eval()) and pass control to DEFINE_TEST_RUNNER_RULES.
diff --git a/tools/testing/selftests/bpf/prog_tests/static_linked.c b/tools/testing/selftests/bpf/prog_tests/static_linked.c
index 46556976dccc..1e6701483d27 100644
--- a/tools/testing/selftests/bpf/prog_tests/static_linked.c
+++ b/tools/testing/selftests/bpf/prog_tests/static_linked.c
@@ -6,7 +6,7 @@ 
 
 void test_static_linked(void)
 {
-	int err;
+	int err, key = 0, value = 0;
 	struct test_static_linked* skel;
 
 	skel = test_static_linked__open();
@@ -35,6 +35,10 @@  void test_static_linked(void)
 	ASSERT_EQ(skel->bss->var1, 1 * 2 + 2 + 3, "var1");
 	ASSERT_EQ(skel->bss->var2, 4 * 3 + 5 + 6, "var2");
 
+	err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.legacy_map), &key, &value);
+	ASSERT_OK(err, "legacy_map_lookup");
+	ASSERT_EQ(value, 1 * 3 + 3,  "legacy_map_value");
+
 cleanup:
 	test_static_linked__destroy(skel);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_static_linked3.nobtf.c b/tools/testing/selftests/bpf/progs/test_static_linked3.nobtf.c
new file mode 100644
index 000000000000..e5fbde21381c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_static_linked3.nobtf.c
@@ -0,0 +1,36 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+/* global variables don't need BTF to be used, but are extremely unconvenient
+ * to be consumed from user-space without BPF skeleton, that uses BTF
+ */
+
+static volatile int mul3 = 3;
+static volatile int add3 = 3;
+
+/* same "subprog" name in all files */
+static __noinline int subprog(int x)
+{
+	/* but different formula */
+	return x * mul3 + add3;
+}
+
+struct bpf_map_def SEC("maps") legacy_map = {
+	.type = BPF_MAP_TYPE_ARRAY,
+	.key_size = sizeof(int),
+	.value_size = sizeof(int),
+	.max_entries = 1,
+};
+
+SEC("raw_tp/sys_enter")
+int handler3(const void *ctx)
+{
+	int key = 0, value = subprog(1);
+
+	bpf_map_update_elem(&legacy_map, &key, &value, BPF_ANY);
+
+	return 0;
+}