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
[off-list ref] wrote:

On Sat, Aug 28, 2021 at 12:40:17PM -0400, Dave Marchevsky wrote:

quoted

On 8/28/21 1:20 AM, Dave Marchevsky wrote:

quoted

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