| Message ID | 20240507133147.24380-1-jose.marchesi@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,V2] bpf: avoid UB in usages of the __imm_insn macro | expand |
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@oracle.com> > Cc: david.faust@oracle.com > Cc: cupertino.miranda@oracle.com > Cc: Yonghong Song <yonghong.song@linux.dev> > Cc: Eduard Zingerman <eddyz87@gmail.com> > --- > 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
> 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? I think you are right, in these particular functions, since the result of the memory read cannot be discarded as the asm uses it. > >> >> Tested in master bpf-next. >> No regressions. >> >> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com> >> Cc: david.faust@oracle.com >> Cc: cupertino.miranda@oracle.com >> Cc: Yonghong Song <yonghong.song@linux.dev> >> Cc: Eduard Zingerman <eddyz87@gmail.com> >> --- >> 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. Ok. I will send a V2 as soon as Cupertino's patch adding support for -bpf_gcc-CFLAGS gets applied upstream. Thanks. > >> +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
> 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@oracle.com> >> Cc: david.faust@oracle.com >> Cc: cupertino.miranda@oracle.com >> Cc: Yonghong Song <yonghong.song@linux.dev> >> Cc: Eduard Zingerman <eddyz87@gmail.com> >> --- >> 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? Sorry, I missed this question. cpumask_failure.c wasn't meant to be there. Will omit it in the V2 of the patch. > > 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
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 +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
[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. Tested in master bpf-next. No regressions. Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com> Cc: david.faust@oracle.com Cc: cupertino.miranda@oracle.com Cc: Yonghong Song <yonghong.song@linux.dev> Cc: Eduard Zingerman <eddyz87@gmail.com> --- tools/testing/selftests/bpf/Makefile | 15 +++++++++++++++ 1 file changed, 15 insertions(+)