On Sun, Aug 29, 2021 at 9:57 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> 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@xxxxxx>
> > > ---
> > > 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")
