| Message ID | 20210828052006.1313788-5-davemarchevsky@fb.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bpf: implement variadic printk helper | expand |
On Sat, Aug 28, 2021 at 12:40:17PM -0400, Dave Marchevsky wrote: > On 8/28/21 1:20 AM, Dave Marchevsky wrote: > > The __bpf_printk convenience macro was using a 'char' fmt string holder > > as it predates support for globals in libbpf. Move to more efficient > > 'static const char', but provide a fallback to the old way via > > BPF_NO_GLOBAL_DATA so users on old kernels can still use the macro. > > > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > > --- > > tools/lib/bpf/bpf_helpers.h | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > > index 5f087306cdfe..a1d5ec6f285c 100644 > > --- a/tools/lib/bpf/bpf_helpers.h > > +++ b/tools/lib/bpf/bpf_helpers.h > > @@ -216,10 +216,16 @@ enum libbpf_tristate { > > ___param, sizeof(___param)); \ > > }) > > > > +#ifdef BPF_NO_GLOBAL_DATA > > +#define BPF_PRINTK_FMT_TYPE char > > +#else > > +#define BPF_PRINTK_FMT_TYPE static const char > > The reference_tracking prog test is failing as a result of this. > Specifically, it fails to load bpf_sk_lookup_test0 prog, which > has a bpf_printk: > > 47: (b4) w3 = 0 > 48: (18) r1 = 0x0 > 50: (b4) w2 = 7 > 51: (85) call bpf_trace_printk#6 > R1 type=inv expected=fp, pkt, pkt_meta, map_key, map_value, mem, rdonly_buf, rdwr_buf > > Setting BPF_NO_GLOBAL_DATA in the test results in a pass hmm. that's odd. pls investigate. Worst case we can just drop this patch for now. The failing printk is this one, right? bpf_printk("sk=%d\n", sk ? 1 : 0); iirc we had an issue related to ?: operand being used as an argument and llvm generating interesting code path with 'sk' and the later if (sk) bpf_sk_release(sk); would not be properly recognized by the verifier leading it to believe that sk may not be released in some cases. That printk was triggering such interesting llvm codegen. See commit d844a71bff0f ("bpf: Selftests, add printk to test_sk_lookup_kern to encode null ptr check")
On Sun, Aug 29, 2021 at 9:57 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Sat, Aug 28, 2021 at 12:40:17PM -0400, Dave Marchevsky wrote: > > On 8/28/21 1:20 AM, Dave Marchevsky wrote: > > > The __bpf_printk convenience macro was using a 'char' fmt string holder > > > as it predates support for globals in libbpf. Move to more efficient > > > 'static const char', but provide a fallback to the old way via > > > BPF_NO_GLOBAL_DATA so users on old kernels can still use the macro. > > > > > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > > > --- > > > tools/lib/bpf/bpf_helpers.h | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > > > index 5f087306cdfe..a1d5ec6f285c 100644 > > > --- a/tools/lib/bpf/bpf_helpers.h > > > +++ b/tools/lib/bpf/bpf_helpers.h > > > @@ -216,10 +216,16 @@ enum libbpf_tristate { > > > ___param, sizeof(___param)); \ > > > }) > > > > > > +#ifdef BPF_NO_GLOBAL_DATA > > > +#define BPF_PRINTK_FMT_TYPE char > > > +#else > > > +#define BPF_PRINTK_FMT_TYPE static const char > > > > The reference_tracking prog test is failing as a result of this. > > Specifically, it fails to load bpf_sk_lookup_test0 prog, which > > has a bpf_printk: > > > > 47: (b4) w3 = 0 > > 48: (18) r1 = 0x0 > > 50: (b4) w2 = 7 > > 51: (85) call bpf_trace_printk#6 > > R1 type=inv expected=fp, pkt, pkt_meta, map_key, map_value, mem, rdonly_buf, rdwr_buf > > > > Setting BPF_NO_GLOBAL_DATA in the test results in a pass > > hmm. that's odd. pls investigate. It's a broken reference_tracking selftest which uses direct calls into bpf_program__load() API, which is not supposed to be used directly. In this case bpf_program__load() doesn't apply any relocation for .rodata, so verifier rightfully complains that constant zero is not really a valid pointer to memory. It's a plan for libbpf 1.0 to hide bpf_program__load() (which is supposed to be used only internally by libbpf). And it's surprising that we have a test using that API directly, it somehow slipped by us. Dave, can you please switch this selftest to use bpf_object__load() properly? This seems to be the only selftests that's using bpf_program__load(). You'll probably need to open/iterate programs/bpf_progam__set_autoload() properly based on name/bpf_object__load() in a loop for each BPF prog to be tested. > Worst case we can just drop this patch for now. > The failing printk is this one, right? > bpf_printk("sk=%d\n", sk ? 1 : 0); > iirc we had an issue related to ?: operand being used as an argument > and llvm generating interesting code path with 'sk' and the later > if (sk) bpf_sk_release(sk); > would not be properly recognized by the verifier leading it to > believe that sk may not be released in some cases. > That printk was triggering such interesting llvm codegen. > See commit d844a71bff0f ("bpf: Selftests, add printk to test_sk_lookup_kern to encode null ptr check")
On Fri, Aug 27, 2021 at 10:20 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > The __bpf_printk convenience macro was using a 'char' fmt string holder > as it predates support for globals in libbpf. Move to more efficient > 'static const char', but provide a fallback to the old way via > BPF_NO_GLOBAL_DATA so users on old kernels can still use the macro. > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > --- > tools/lib/bpf/bpf_helpers.h | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > index 5f087306cdfe..a1d5ec6f285c 100644 > --- a/tools/lib/bpf/bpf_helpers.h > +++ b/tools/lib/bpf/bpf_helpers.h > @@ -216,10 +216,16 @@ enum libbpf_tristate { > ___param, sizeof(___param)); \ > }) > > +#ifdef BPF_NO_GLOBAL_DATA > +#define BPF_PRINTK_FMT_TYPE char > +#else > +#define BPF_PRINTK_FMT_TYPE static const char > +#endif > + > /* Helper macro to print out debug messages */ > #define __bpf_printk(fmt, ...) \ > ({ \ > - char ____fmt[] = fmt; \ > + BPF_PRINTK_FMT_TYPE ____fmt[] = fmt; \ personal preferences, of course, but I'd leave char right there (I think it makes it a bit more obvious what's going on right there), and s/BPF_PRINTK_FMT_TYPE/BPF_PRINTK_FMT_MOD/ and have it as either "" or "static const". > bpf_trace_printk(____fmt, sizeof(____fmt), \ > ##__VA_ARGS__); \ > }) > -- > 2.30.2 >
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h index 5f087306cdfe..a1d5ec6f285c 100644 --- a/tools/lib/bpf/bpf_helpers.h +++ b/tools/lib/bpf/bpf_helpers.h @@ -216,10 +216,16 @@ enum libbpf_tristate { ___param, sizeof(___param)); \ }) +#ifdef BPF_NO_GLOBAL_DATA +#define BPF_PRINTK_FMT_TYPE char +#else +#define BPF_PRINTK_FMT_TYPE static const char +#endif + /* Helper macro to print out debug messages */ #define __bpf_printk(fmt, ...) \ ({ \ - char ____fmt[] = fmt; \ + BPF_PRINTK_FMT_TYPE ____fmt[] = fmt; \ bpf_trace_printk(____fmt, sizeof(____fmt), \ ##__VA_ARGS__); \ })
The __bpf_printk convenience macro was using a 'char' fmt string holder as it predates support for globals in libbpf. Move to more efficient 'static const char', but provide a fallback to the old way via BPF_NO_GLOBAL_DATA so users on old kernels can still use the macro. Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> --- tools/lib/bpf/bpf_helpers.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)