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: <20210829165714.wghn236g2ka7lgna@ast-mbp.dhcp.thefacebook.com>
Date:   Sun, 29 Aug 2021 09:57:14 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Dave Marchevsky <davemarchevsky@...com>
Cc:     bpf@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Yonghong Song <yhs@...com>, netdev@...r.kernel.org
Subject: Re: [PATCH v3 bpf-next 4/7] libbpf: use static const fmt string in
 __bpf_printk

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.
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")