netdev - Re: [PATCH v3 bpf-next 4/7] libbpf: use static const fmt string in __bpf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4Bzar9-ZLeponMHuU3ALUPGv4eKS-bObPzu_HO8BC+2KrQg@mail.gmail.com>
Date:   Mon, 30 Aug 2021 17:36:01 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Dave Marchevsky <davemarchevsky@...com>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Yonghong Song <yhs@...com>, Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 bpf-next 4/7] libbpf: use static const fmt string in __bpf_printk

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