| Message ID | 20210319205909.1748642-4-andrii@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Handle no-BTF object files better | expand |
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.
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.
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?
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?
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.
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.
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.
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.
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.
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 --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; +}
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