| Message ID | 20211008000309.43274-1-andrii@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | libbpf: support custom .rodata.*/.data.* sections | expand |
On 10/7/21 5:02 PM, andrii.nakryiko@gmail.com wrote: > > One interesting possibility that is now open by these changes is that it's > possible to do: > > bpf_trace_printk("My fmt %s", sizeof("My fmt %s"), "blah"); > > and it will work as expected. Could you explain what would be the difference vs existing bpf_printk macro? Why these patches enable above to work ? > I haven't updated libbpf-provided helpers in > bpf_helpers.h for snprintf, seq_printf, and printk, because using > `static const char ___fmt[] = fmt;` trick is still efficient and doesn't fill > out the buffer at runtime (no copying), but it also enforces that format > string is compile-time string literal: > > const char *s = NULL; > > bpf_printk("hi"); /* works */ > bpf_printk(s); /* compilation error */ > > By passing fmt directly to bpf_trace_printk() would actually compile > bpf_printk(s) above with no warnings and will not work at runtime, which is > worse user experience, IMO. What is the example of "compile with no warning and not work at runtime" ?
On Mon, Oct 11, 2021 at 11:30 PM Alexei Starovoitov <ast@fb.com> wrote: > > On 10/7/21 5:02 PM, andrii.nakryiko@gmail.com wrote: > > > > One interesting possibility that is now open by these changes is that it's > > possible to do: > > > > bpf_trace_printk("My fmt %s", sizeof("My fmt %s"), "blah"); > > > > and it will work as expected. > > Could you explain what would be the difference vs existing bpf_printk macro? > Why these patches enable above to work ? Good question. Have you ever noticed warnings like this during selftests build: libbpf: elf: skipping unrecognized data section(6) .rodata.str1.1 I'm recalling from memory a bit here, so excuse me if I get some small details wrong. Let's say we have a `bpf_printk("Hello!\n");` call. It is turned into: static const char fmt[] = "Hello!\n"; bpf_trace_printk(fmt, 7); `fmt` variable above is always in .rodata section, it will even have a dedicated BTF VAR with the name '<func>.fmt'. For reasons unknown to me (Yonghong will know, probably), the compiler *also* and *sometimes* puts the same "Hello!\n" string into the .rodata.str1.1 section. So we end up with this warning about unknown and skipped .rodata.str1.1. Good news is that we actually never reference "Hello!\n" from .rodata.str1.1, which is why the bpf_printk() works today. But if you were to rewrite the above snippet as more natural: bpf_trace_printk("Hello!\n", 7); ... and compiler puts that "Hello!\n" into .rodata.str1.1 (and *not* into .rodata), then we'd have a relocation against .rodata.str1.1, which will fail because up until now libbpf never did anything with .rodata.str1.1. So it's a bit roundabout way to say that with this patch set, no matter which .rodata* section compiler decided to put compile-time constants into (doesn't have to be string, could be struct literal for initializing a struct, for example) it should work, because it's just a normal relocation, so all the libbpf relocation logic works, and for kernel it will be just another global data ARRAY, so everything works as expected. What was necessary to make this work was internal refactoring to remove a simplifying assumption that there could be only one .rodata map, and one .data map, etc. Hope that clears it a bit. > > > I haven't updated libbpf-provided helpers in > > bpf_helpers.h for snprintf, seq_printf, and printk, because using > > `static const char ___fmt[] = fmt;` trick is still efficient and doesn't fill > > out the buffer at runtime (no copying), but it also enforces that format > > string is compile-time string literal: > > > > const char *s = NULL; > > > > bpf_printk("hi"); /* works */ > > bpf_printk(s); /* compilation error */ > > > > By passing fmt directly to bpf_trace_printk() would actually compile > > bpf_printk(s) above with no warnings and will not work at runtime, which is > > worse user experience, IMO. > > What is the example of "compile with no warning and not work at runtime" ? Right, so imagine we rewritten bpf_printk to expand into bpf_trace_printk(fmt, sizeof(fmt), args...)); Now, if you do bpf_printk("BLAH"), everything works, fmt string literal is passed properly and its size is calculated as 5. But if you now pass const char *, you end up doing: bpf_trace_printk(s, sizeof(s) /* == 8 */, args...); See how passing a pointer is right for the first argument, but using it to determine the string size is completely wrong. And the worst thing is it won't trigger any warning. There's a similar danger of using sizeof() with arrays. Pure C language gotcha, nothing BPF specific.
On Fri, Oct 08, 2021 at 05:32:59AM IST, andrii.nakryiko@gmail.com wrote: > From: Andrii Nakryiko <andrii@kernel.org> > > [...] > One interesting possibility that is now open by these changes is that it's > possible to do: > > bpf_trace_printk("My fmt %s", sizeof("My fmt %s"), "blah"); > > and it will work as expected. I haven't updated libbpf-provided helpers in > bpf_helpers.h for snprintf, seq_printf, and printk, because using > `static const char ___fmt[] = fmt;` trick is still efficient and doesn't fill > out the buffer at runtime (no copying), but it also enforces that format > string is compile-time string literal: > > const char *s = NULL; > > bpf_printk("hi"); /* works */ > bpf_printk(s); /* compilation error */ > > By passing fmt directly to bpf_trace_printk() would actually compile > bpf_printk(s) above with no warnings and will not work at runtime, which is > worse user experience, IMO. > You could try the following (_Static_assert would probably be preferable, but IDK if libbpf can use it). #define IS_ARRAY(x) ({ sizeof(int[-__builtin_types_compatible_p(typeof(x), typeof(&*(x)))]); 1; }) In action: https://godbolt.org/z/4d4W61YPf -- Kartikeya
On Mon, Oct 11, 2021 at 9:15 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Fri, Oct 08, 2021 at 05:32:59AM IST, andrii.nakryiko@gmail.com wrote: > > From: Andrii Nakryiko <andrii@kernel.org> > > > > [...] > > One interesting possibility that is now open by these changes is that it's > > possible to do: > > > > bpf_trace_printk("My fmt %s", sizeof("My fmt %s"), "blah"); > > > > and it will work as expected. I haven't updated libbpf-provided helpers in > > bpf_helpers.h for snprintf, seq_printf, and printk, because using > > `static const char ___fmt[] = fmt;` trick is still efficient and doesn't fill > > out the buffer at runtime (no copying), but it also enforces that format > > string is compile-time string literal: > > > > const char *s = NULL; > > > > bpf_printk("hi"); /* works */ > > bpf_printk(s); /* compilation error */ > > > > By passing fmt directly to bpf_trace_printk() would actually compile > > bpf_printk(s) above with no warnings and will not work at runtime, which is > > worse user experience, IMO. > > > > You could try the following (_Static_assert would probably be preferable, but > IDK if libbpf can use it). Yeah, we definitely can use _Static_assert from BPF side (see progs/test_cls_redirect.c in selftests/bpf). > > #define IS_ARRAY(x) ({ sizeof(int[-__builtin_types_compatible_p(typeof(x), typeof(&*(x)))]); 1; }) > Thanks! This seems to be working well to detect arrays and string literals. I'll keep it in my toolbox :) Ultimately I decided to not touch bpf_printk() for now, because of the BPF_NO_GLOBAL_DATA trick. If I go with direct "fmt" usage, I'll need to implementations of __bpf_printk(), which doesn't seem worth it right now. I might revisit it later, though. > In action: https://godbolt.org/z/4d4W61YPf > > -- > Kartikeya
From: Andrii Nakryiko <andrii@kernel.org> This patch set refactors internals of libbpf to enable support for multiple custom .rodata.* and .data.* sections. Each such section is backed by its own BPF_MAP_TYPE_ARRAY, memory-mappable just like .rodat/.data. This is not extended to .bss because .bss is not a great name, it is generated by compiler with name that reflects completely irrelevant historical implementation details. Given that users have to annotate their variables with SEC(".data.my_sec") explicitly, standardizing on .rodata. and .data. prefixes makes more sense and keeps things simpler. Additionally, this patch set makes it simpler to work with those special internal maps by allowing to look them up by their full ELF section name. Patch #1 is a preparatory patch that deprecated one libbpf API and moved custom logic into libbpf.c where it's used. This code is later refactored with the rest of libbpf.c logic to support multiple data section maps. See individual patches for all the details. One open question I have is whether we want to preserve this convoluted logic of concatenating BPF object name with ELF section name for new custom data sections/maps. Given their names are always going to be pretty long, it might not make sense to drag this convention along and have kernel-side map name differ from user-visible map name. See patch #7 for details on this. One interesting possibility that is now open by these changes is that it's possible to do: bpf_trace_printk("My fmt %s", sizeof("My fmt %s"), "blah"); and it will work as expected. I haven't updated libbpf-provided helpers in bpf_helpers.h for snprintf, seq_printf, and printk, because using `static const char ___fmt[] = fmt;` trick is still efficient and doesn't fill out the buffer at runtime (no copying), but it also enforces that format string is compile-time string literal: const char *s = NULL; bpf_printk("hi"); /* works */ bpf_printk(s); /* compilation error */ By passing fmt directly to bpf_trace_printk() would actually compile bpf_printk(s) above with no warnings and will not work at runtime, which is worse user experience, IMO. But there could be other interesting uses for non-trivial compile-time constants nevertheless. Andrii Nakryiko (10): libbpf: deprecate btf__finalize_data() and move it into libbpf.c libbpf: extract ELF processing state into separate struct libbpf: use Elf64-specific types explicitly for dealing with ELF libbpf: remove assumptions about uniqueness of .rodata/.data/.bss maps bpftool: support multiple .rodata/.data internal maps in skeleton bpftool: improve skeleton generation for data maps without DATASEC type libbpf: support multiple .rodata.* and .data.* BPF maps selftests/bpf: demonstrate use of custom .rodata/.data sections libbpf: simplify look up by name of internal maps selftests/bpf: switch to ".bss"/".rodata"/".data" lookups for internal maps tools/bpf/bpftool/gen.c | 158 ++-- tools/lib/bpf/btf.c | 93 -- tools/lib/bpf/btf.h | 1 + tools/lib/bpf/libbpf.c | 887 +++++++++++------- tools/lib/bpf/libbpf_internal.h | 8 +- tools/lib/bpf/linker.c | 1 - .../selftests/bpf/prog_tests/core_autosize.c | 2 +- .../selftests/bpf/prog_tests/core_reloc.c | 2 +- .../selftests/bpf/prog_tests/global_data.c | 11 +- .../bpf/prog_tests/global_data_init.c | 2 +- .../selftests/bpf/prog_tests/kfree_skb.c | 2 +- .../selftests/bpf/prog_tests/rdonly_maps.c | 2 +- .../selftests/bpf/prog_tests/skeleton.c | 25 + .../selftests/bpf/progs/test_skeleton.c | 10 + 14 files changed, 713 insertions(+), 491 deletions(-)