[RFC][clang][BPF] Make trivial uninit var value to be 0

[yonghong-song]

Marc Suñé (Isovalent, part of Cisco) reported an issue where an uninitialized variable caused generated bpf prog binary code not working as expected. The reproducer is in [1] where the flags “-Wall -Werror” are enabled, but there is no warning and compiler may take advantage of uninit variable to do aggressive optimization.

In the BPF context, the generated buggy code will be load into the kernel and likely verification will pass. When the bpf program actually runs, its result may not be expected and user will then need to check source or asm code to find reasons. Since compiler may take advantage of uninit variables for aggressive optimization, users may be confused with final code without realizing this is due to uninit variables.

The ideal case is for compiler to issue meaning info about actual uninit variable which impacts transformation. Currently clang (and gcc) may miss such uninit variables, esp. with cross function boundaries. See discuss in [2].

The llvm/gcc undef sanitizer is another source to detect such undef issues. For bpf target, this means potential complexity for verifier and bpf runtime.

Another idea is to make -ftrivial-auto-var-init=0 as the default for bpf target. This way, when bpf prog produced unexpected results, users can check/understand asm codes easier since compiler won't be able to do aggressive optimization due to uninit variables.

This patch makes -ftrivial-auto-var-init=0 as the default for bpf target. For example, for code,

  int foo1() {
    int val;
    return val;
  }
With this patch, the eventual IR bofore lowering is
  ret i32 0;
Without this patch, it is
  ret i32 undef

With -ftrivial-auto-var-init=0 on by default, two bpf selftests failed and below the fix:

  diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
  index bd8f15229f5c..72a65ee365ef 100644
  --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
  +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
  @@ -413,7 +413,11 @@ int invalid_helper1(void *ctx)

   /* A dynptr can't be passed into a helper function at a non-zero offset */
   SEC("?raw_tp")
  +#if __clang_major__ >= 21
  +__failure __msg("Expected an initialized dynptr as arg #2")
  +#else
   __failure __msg("cannot pass in dynptr at an offset=-8")
  +#endif
   int invalid_helper2(void *ctx)
   {
          struct bpf_dynptr ptr;
  diff --git a/tools/testing/selftests/bpf/progs/verifier_mtu.c b/tools/testing/selftests/bpf/progs/verifier_mtu.c
  index 256956ea1ac5..c987b20b39eb 100644
  --- a/tools/testing/selftests/bpf/progs/verifier_mtu.c
  +++ b/tools/testing/selftests/bpf/progs/verifier_mtu.c
  @@ -8,7 +8,9 @@ SEC("tc/ingress")
   __description("uninit/mtu: write rejected")
   __success
   __caps_unpriv(CAP_BPF|CAP_NET_ADMIN)
  +#if __clang_major__ < 21
   __failure_unpriv __msg_unpriv("invalid read from stack")
  +#endif
   int tc_uninit_mtu(struct __sk_buff *ctx)
   {
          __u32 mtu;

Not sure whether we should introduce a bpf specific flag to disable -ftrivial-auto-var-init=zero or not. I guess probably not. But the kernel change still needed.

[1] https://github.com/msune/clang_bpf/blob/main/Makefile#L3
[2] https://discourse.llvm.org/t/detect-undefined-behavior-due-to-uninitialized-variables-in-bpf-programs/84116?u=yonghong-song

[yonghong-song]

cc @anakryiko @jemarch

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (yonghong-song)

Changes

Marc Suñé (Isovalent, part of Cisco) reported an issue where an uninitialized variable caused generated bpf prog binary code not working as expected. The reproducer is in [1] where the flags “-Wall -Werror” are enabled, but there is no warning and compiler may take advantage of uninit variable to do aggressive optimization.

In the BPF context, the generated buggy code will be load into the kernel and likely verification will pass. When the bpf program actually runs, its result may not be expected and user will then need to check source or asm code to find reasons. Since compiler may take advantage of uninit variables for aggressive optimization, users may be confused with final code without realizing this is due to uninit variables.

The ideal case is for compiler to issue meaning info about actual uninit variable which impacts transformation. Currently clang (and gcc) may miss such uninit variables, esp. with cross function boundaries. See discuss in [2].

The llvm/gcc undef sanitizer is another source to detect such undef issues. For bpf target, this means potential complexity for verifier and bpf runtime.

Another idea is to make -ftrivial-auto-var-init=0 as the default for bpf target. This way, when bpf prog produced unexpected results, users can check/understand asm codes easier since compiler won't be able to do aggressive optimization due to uninit variables.

This patch makes -ftrivial-auto-var-init=0 as the default for bpf target. For example, for code,

  int foo1() {
    int val;
    return val;
  }
With this patch, the eventual IR bofore lowering is
  ret i32 0;
Without this patch, it is
  ret i32 undef

With -ftrivial-auto-var-init=0 on by default, two bpf selftests failed and below the fix:

  diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
  index bd8f15229f5c..72a65ee365ef 100644
  --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
  +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
  @@ -413,7 +413,11 @@ int invalid_helper1(void *ctx)

   /* A dynptr can't be passed into a helper function at a non-zero offset */
   SEC("?raw_tp")
  +#if __clang_major__ >= 21
  +__failure __msg("Expected an initialized dynptr as arg #<!-- -->2")
  +#else
   __failure __msg("cannot pass in dynptr at an offset=-8")
  +#endif
   int invalid_helper2(void *ctx)
   {
          struct bpf_dynptr ptr;
  diff --git a/tools/testing/selftests/bpf/progs/verifier_mtu.c b/tools/testing/selftests/bpf/progs/verifier_mtu.c
  index 256956ea1ac5..c987b20b39eb 100644
  --- a/tools/testing/selftests/bpf/progs/verifier_mtu.c
  +++ b/tools/testing/selftests/bpf/progs/verifier_mtu.c
  @@ -8,7 +8,9 @@ SEC("tc/ingress")
   __description("uninit/mtu: write rejected")
   __success
   __caps_unpriv(CAP_BPF|CAP_NET_ADMIN)
  +#if __clang_major__ &lt; 21
   __failure_unpriv __msg_unpriv("invalid read from stack")
  +#endif
   int tc_uninit_mtu(struct __sk_buff *ctx)
   {
          __u32 mtu;

Not sure whether we should introduce a bpf specific flag to disable -ftrivial-auto-var-init=0 or not. I guess probably not. But the kernel change still needed.

[1] https://github.com/msune/clang_bpf/blob/main/Makefile#L3
[2] https://discourse.llvm.org/t/detect-undefined-behavior-due-to-uninitialized-variables-in-bpf-programs/84116?u=yonghong-song

---

Full diff: https://github.com/llvm/llvm-project/pull/125601.diff

3 Files Affected:

• (modified) clang/lib/Basic/Targets/BPF.cpp (+8)
• (modified) clang/lib/Basic/Targets/BPF.h (+2)
• (added) clang/test/CodeGen/bpf-auto-var-init.c (+30)

diff --git a/clang/lib/Basic/Targets/BPF.cpp b/clang/lib/Basic/Targets/BPF.cpp
index f4684765b7ffb32..231a1dc2457bc7d 100644
--- a/clang/lib/Basic/Targets/BPF.cpp
+++ b/clang/lib/Basic/Targets/BPF.cpp
@@ -81,6 +81,14 @@ void BPFTargetInfo::fillValidCPUList(SmallVectorImpl<StringRef> &Values) const {
   Values.append(std::begin(ValidCPUNames), std::end(ValidCPUNames));
 }
 
+void BPFTargetInfo::adjust(DiagnosticsEngine &Diags, LangOptions &Opts) {
+  TargetInfo::adjust(Diags, Opts);
+
+  if (Opts.getTrivialAutoVarInit() ==
+      LangOptions::TrivialAutoVarInitKind::Uninitialized)
+    Opts.setTrivialAutoVarInit(LangOptions::TrivialAutoVarInitKind::Zero);
+}
+
 ArrayRef<Builtin::Info> BPFTargetInfo::getTargetBuiltins() const {
   return llvm::ArrayRef(BuiltinInfo,
                         clang::BPF::LastTSBuiltin - Builtin::FirstTSBuiltin);
diff --git a/clang/lib/Basic/Targets/BPF.h b/clang/lib/Basic/Targets/BPF.h
index 27a4b5f31497024..b12a09fb2db5c14 100644
--- a/clang/lib/Basic/Targets/BPF.h
+++ b/clang/lib/Basic/Targets/BPF.h
@@ -60,6 +60,8 @@ class LLVM_LIBRARY_VISIBILITY BPFTargetInfo : public TargetInfo {
 
   ArrayRef<Builtin::Info> getTargetBuiltins() const override;
 
+  void adjust(DiagnosticsEngine &Diags, LangOptions &Opts) override;
+
   std::string_view getClobbers() const override { return ""; }
 
   BuiltinVaListKind getBuiltinVaListKind() const override {
diff --git a/clang/test/CodeGen/bpf-auto-var-init.c b/clang/test/CodeGen/bpf-auto-var-init.c
new file mode 100644
index 000000000000000..09023279e1eeeb7
--- /dev/null
+++ b/clang/test/CodeGen/bpf-auto-var-init.c
@@ -0,0 +1,30 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang_cc1 -triple bpf -O2 -emit-llvm %s -o - | FileCheck %s
+
+int foo1() {
+  int val;
+  return val;
+}
+// CHECK: ret i32 0
+
+int foo2() {
+  int val[4];
+  return val[2];
+}
+// CHECK: ret i32 0
+
+struct val_t {
+  int val;
+};
+
+int foo3() {
+  struct val_t v;
+  return v.val;
+}
+// CHECK: ret i32 0
+
+int foo4() {
+  int val = 5;
+  return val;
+}
+// CHECK: ret i32 5

[4ast]

what veristat says before/after?

re: selftests

there should be a flag to disable this, so the tests can remain as-is (with only Makefile change).

There is also -ftrivial-auto-var-init-max-size= flag. What is the default?
Looks like: char buf[64]; will be zero inited as well?
That will probably hurt performance/verification for cases like:
char comm[16];
bpf_get_current_comm(comm, ...)
?

[yonghong-song]

what veristat says before/after?

I tried with latest bpf-next and latest llvm trunk. The following is the difference
with/without this patch:

[~/work/bpf-next/tools/testing/selftests/bpf (master)]$ ./veristat *.bpf.o -o csv > old.csv
[~/work/bpf-next/tools/testing/selftests/bpf (master)]$ ./veristat *.bpf.o -o csv > new.csv
[~/work/bpf-next/tools/testing/selftests/bpf (master)]$ ./veristat -e file,prog,insns -C old.csv new.csv -f 'insns_diff!=0'
File                             Program                             Insns (A)  Insns (B)  Insns  (DIFF)
-------------------------------  ----------------------------------  ---------  ---------  -------------
cgroup_skb_sk_lookup_kern.bpf.o  ingress_lookup                             96        106  +10 (+10.42%)
cgroup_tcp_skb.bpf.o             client_egress                              94        104  +10 (+10.64%)
cgroup_tcp_skb.bpf.o             client_egress_srv                         115        125   +10 (+8.70%)
cgroup_tcp_skb.bpf.o             client_ingress                            115        125   +10 (+8.70%)
cgroup_tcp_skb.bpf.o             client_ingress_srv                         94        104  +10 (+10.64%)
cgroup_tcp_skb.bpf.o             server_egress                             115        125   +10 (+8.70%)
cgroup_tcp_skb.bpf.o             server_egress_srv                          94        104  +10 (+10.64%)
cgroup_tcp_skb.bpf.o             server_ingress                            129        139   +10 (+7.75%)
cgroup_tcp_skb.bpf.o             server_ingress_srv                        137        147   +10 (+7.30%)
verifier_array_access.bpf.o      an_array_with_a_constant_too_small          7          9   +2 (+28.57%)
[~/work/bpf-next/tools/testing/selftests/bpf (master)]$

For cgroup_tcp_skb.c, e.g.,

SEC("cgroup_skb/egress")
int server_egress_srv(struct __sk_buff *skb)
{
        struct tcphdr tcph;

        if (!needed_tcp_pkt(skb, &tcph))
                return 1;
...

the tcphdr is larger so it costs more with initializing to 0.
The same for cgroup_skb_sk_lookup_kern.c.

For verifier_array_access.c,

SEC("socket")
__description("invalid map access into an array using constant smaller than key_size")
__failure __msg("R0 invalid mem access 'map_value_or_null'")
unsigned int an_array_with_a_constant_too_small(void)
{       
        __u32 __attribute__((aligned(8))) key;
        struct test_val *val;

        /* Mark entire key as STACK_MISC */
        bpf_probe_read_user(&key, sizeof(key), NULL);
        ...
}

the 'key' is initialized.

re: selftests

there should be a flag to disable this, so the tests can remain as-is (with only Makefile change).

Will do.

There is also -ftrivial-auto-var-init-max-size= flag. What is the default? Looks like: char buf[64]; will be zero inited as well?

The default is 0 (-ftrivial-auto-var-init-max-size=0) which means there is no limitation.
yes, char buf[64] and all buf[64] will be initialized to 0 if the compiler does not know whehter any buf[i](i = 0...63) is used or not.

That will probably hurt performance/verification for cases like: char comm[16]; bpf_get_current_comm(comm, ...) ?
I checked. In almost all cases in bpf selftest progs, comm[16] is a field in a map, so it is not impacted.

[4ast]

all buf[64] will be initialized to 0

looks like it's a show stopper. The performance impact of initializing everything might be felt for XDP.

[eddyz87]

I looked at the example shared by Yonghong, the change clang does because of the uninitialized variable access is indeed quite dramatic. It computes a switch over undef and throws away all branches including "default". Basically throwing away most of the program.

Nevertheless, the issue is not really solved by using zero initialization, as that is not what programmer intended and might be equally as hard to debug. And this is not limited to BPF backend. If we really consider this issue to be a priority for BPF backend, I think we should invest time in better semantic warnings, or otherwise leave things as is.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[yonghong-song]

I just updated a patch to change TrivialAutoVarInitMaxSize to 8 if TrivialAutoVarInit is set to Zero by clang bpf target. This should mostly solve the issue for xdp programs.

@eddyz87 suggested to improve middle-end compilation to issue proper warnings. This is what I initially wanted to do as well but feel it would be quite complicated esp. related to cross-functions. I am not sure how cross-file checking can be done for bpf programs since effectively, we do not do IR-level merge across files.

One thing I tried earlier is to do -fsanitize=undefined for bpf programs (see discourse discussion link in the description). There will be a 'undef' IR asm code to specify as undefined behavior at particular place. Current bpf backend maps 'undef' to a no-op.
What we could do is that bpf backend could map 'undef' to a BPF 'trap' insn. Later, if verifier hits a 'trap' insn, verification should fail. In case verifier won't be able to pass due to 'trap' insn even if the prog is legal, 'trap' insn can only be issued if -fsanitize=undefined is specified by user. WDYT?

[4ast]

I agree with Eduard. I think we should abandon this patch and make -fsanitize=undefined work.
undef can be mapped to a new bpf_fastcall kfunc no args and void return, so no extra register pressure and call can be inserted. That kfunc can eventually abort prog execution and print into upcoming per-prog trace_pipe
(which we're adding to support deadlock reports, and aborts on page faults)

[yonghong-song]

Okay, I will work on -fsanitize=undefined for bpf then.