On 5/7/24 6:31 AM, Jose E. Marchesi wrote:
[Differences with V1:
- Typo fixed in patch: progs/verifier_ref_tracking.c
was missing -CFLAGS.]
The __imm_insn macro is defined in bpf_misc.h as:
#define __imm_insn(name, expr) [name]"i"(*(long *)&(expr))
This may lead to type-punning and strict aliasing rules violations in
it's typical usage where the address of a struct bpf_insn is passed as
expr, like in:
__imm_insn(st_mem,
BPF_ST_MEM(BPF_W, BPF_REG_1, offsetof(struct __sk_buff, mark), 42))
Where:
#define BPF_ST_MEM(SIZE, DST, OFF, IMM) \
((struct bpf_insn) { \
.code = BPF_ST | BPF_SIZE(SIZE) | BPF_MEM, \
.dst_reg = DST, \
.src_reg = 0, \
.off = OFF, \
.imm = IMM })
GCC detects this problem (indirectly) by issuing a warning stating
that a temporary <Uxxxxxx> is used uninitialized, where the temporary
corresponds to the memory read by *(long *).
This patch adds -fno-strict-aliasing to the compilation flags of the
particular selftests that do type punning via __imm_insn. This
silences the warning and, most importantly, avoids potential
optimization problems due to breaking anti-aliasing rules.
For all the modified verifier_* files below, the functions
are naked inline asm, so there is no optimization risk of breaking
anti-aliasing rules. Is this right?
Tested in master bpf-next.
No regressions.
Signed-off-by: Jose E. Marchesi <jose.marchesi@xxxxxxxxxx>
Cc: david.faust@xxxxxxxxxx
Cc: cupertino.miranda@xxxxxxxxxx
Cc: Yonghong Song <yonghong.song@xxxxxxxxx>
Cc: Eduard Zingerman <eddyz87@xxxxxxxxx>
---
tools/testing/selftests/bpf/Makefile | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index f0c429cf4424..c7507f420d9e 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -53,6 +53,21 @@ progs/syscall.c-CFLAGS := -fno-strict-aliasing
progs/test_pkt_md_access.c-CFLAGS := -fno-strict-aliasing
progs/test_sk_lookup.c-CFLAGS := -fno-strict-aliasing
progs/timer_crash.c-CFLAGS := -fno-strict-aliasing
+# In the following tests the strict aliasing rules are broken by the
+# __imm_insn macro, that do type-punning from `struct bpf_insn' to
+# long and then uses the value. This triggers an "is used
+# uninitialized" warning in GCC. This in theory may also lead to
+# broken programs, so it is better to disable strict aliasing than
+# inhibiting the warning.
+progs/verifier_ref_tracking.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_unpriv.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_cgroup_storage.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_ld_ind.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_map_ret_val.c-CFLAGS := -fno-strict-aliasing
+progs/cpumask_failure.c-CFLAGS := -fno-strict-aliasing
All these verifier_* files have __imm_insn, but I didn't see
__imm_insn usage for cpumask_failure.c. Did I miss anything?
All these verifier_* files are naked inline asm. So it should not
cause any issues with -fstrict-aliasing. Since there are no
issues for clang. Maybe just add -fno-strict-aliasing for gcc
only to silence the warning.
+progs/verifier_spill_fill.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_subprog_precision.c-CFLAGS := -fno-strict-aliasing
+progs/verifier_uninit.c-CFLAGS := -fno-strict-aliasing
ifneq ($(LLVM),)
# Silence some warnings when compiled with clang
