> On Sat, Dec 31, 2022 at 8:31 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>>
>> BPF has two documented (non-atomic) memory store instructions:
>>
>> BPF_STX: *(size *) (dst_reg + off) = src_reg
>> BPF_ST : *(size *) (dst_reg + off) = imm32
>>
>> Currently LLVM BPF back-end does not emit BPF_ST instruction and does
>> not allow one to be specified as inline assembly.
>>
>> Recently I've been exploring ways to port some of the verifier test
>> cases from tools/testing/selftests/bpf/verifier/*.c to use inline assembly
>> and machinery provided in tools/testing/selftests/bpf/test_loader.c
>> (which should hopefully simplify tests maintenance).
>> The BPF_ST instruction is popular in these tests: used in 52 of 94 files.
>>
>> While it is possible to adjust LLVM to only support BPF_ST for inline
>> assembly blocks it seems a bit wasteful. This patch-set contains a set
>> of changes to verifier necessary in case when LLVM is allowed to
>> freely emit BPF_ST instructions (source code is available here [1]).
>
> Would we gate LLVM's emitting of BPF_ST for C code behind some new
> cpu=v4? What is the benefit for compiler to start automatically emit
> such instructions? Such thinking about logistics, if there isn't much
> benefit, as BPF application owner I wouldn't bother enabling this
> behavior risking regressions on old kernels that don't have these
> changes.
Hmm, GCC happily generates BPF_ST instructions:
$ echo 'int v; void foo () { v = 666; }' | bpf-unknown-none-gcc -O2 -xc -S -o foo.s -
$ cat foo.s
.file "<stdin>"
.text
.align 3
.global foo
.type foo, @function
foo:
lddw %r0,v
stw [%r0+0],666
exit
.size foo, .-foo
.global v
.type v, @object
.lcomm v,4,4
.ident "GCC: (GNU) 12.0.0 20211206 (experimental)"
Been doing that since October 2019, I think before the cpu versioning
mechanism was got in place?
We weren't aware this was problematic. Does the verifier reject such
instructions?
> So I feel like the biggest benefit is to be able to use this
> instruction in embedded assembly, to make writing and maintaining
> tests easier.
>
>> The changes include:
>> - update to verifier.c:check_stack_write_*() functions to track
>> constant values spilled to stack via BPF_ST instruction in a same
>> way stack spills of known registers by BPF_STX are tracked;
>> - updates to verifier.c:convert_ctx_access() and it's callbacks to
>> handle BPF_ST instruction in a way similar to BPF_STX;
>> - some test adjustments and a few new tests.
>>
>> With the above changes applied and LLVM from [1] all test_verifier,
>> test_maps, test_progs and test_progs-no_alu32 test cases are passing.
>>
>> When built using the LLVM version from [1] BPF programs generated for
>> selftests and Cilium programs (taken from [2]) see certain reduction
>> in size, e.g. below are total numbers of instructions for files with
>> over 5K instructions:
>>
>> File Insns Insns Insns Diff
>> w/o with diff pct
>> BPF_ST BPF_ST
>> cilium/bpf_host.o 44620 43774 -846 -1.90%
>> cilium/bpf_lxc.o 36842 36060 -782 -2.12%
>> cilium/bpf_overlay.o 23557 23186 -371 -1.57%
>> cilium/bpf_xdp.o 26397 25931 -466 -1.77%
>> selftests/core_kern.bpf.o 12359 12359 0 0.00%
>> selftests/linked_list_fail.bpf.o 5501 5302 -199 -3.62%
>> selftests/profiler1.bpf.o 17828 17709 -119 -0.67%
>> selftests/pyperf100.bpf.o 49793 49268 -525 -1.05%
>> selftests/pyperf180.bpf.o 88738 87813 -925 -1.04%
>> selftests/pyperf50.bpf.o 25388 25113 -275 -1.08%
>> selftests/pyperf600.bpf.o 78330 78300 -30 -0.04%
>> selftests/pyperf_global.bpf.o 5244 5188 -56 -1.07%
>> selftests/pyperf_subprogs.bpf.o 5262 5192 -70 -1.33%
>> selftests/strobemeta.bpf.o 17154 16065 -1089 -6.35%
>> selftests/test_verif_scale2.bpf.o 11337 11337 0 0.00%
>>
>> (Instructions are counted by counting the number of instruction lines
>> in disassembly).
>>
>> Is community interested in this work?
>> Are there any omissions in my changes to the verifier?
>>
>> Known issue:
>>
>> There are two programs (one Cilium, one selftest) that exhibit
>> anomalous increase in verifier processing time with this patch-set:
>>
>> File Program Insns (A) Insns (B) Insns (DIFF)
>> ------------------- ----------------------------- --------- --------- --------------
>> bpf_host.o tail_ipv6_host_policy_ingress 1576 2403 +827 (+52.47%)
>> map_kptr.bpf.o test_map_kptr 400 475 +75 (+18.75%)
>> ------------------- ----------------------------- --------- --------- --------------
>>
>> These are under investigation.
>>
>> Thanks,
>> Eduard
>>
>> [1] https://reviews.llvm.org/D140804
>> [2] git@xxxxxxxxxx:anakryiko/cilium.git
>>
>> Eduard Zingerman (5):
>> bpf: more precise stack write reasoning for BPF_ST instruction
>> selftests/bpf: check if verifier tracks constants spilled by
>> BPF_ST_MEM
>> bpf: allow ctx writes using BPF_ST_MEM instruction
>> selftests/bpf: test if pointer type is tracked for BPF_ST_MEM
>> selftests/bpf: don't match exact insn index in expected error message
>>
>> kernel/bpf/cgroup.c | 49 +++++---
>> kernel/bpf/verifier.c | 102 +++++++++-------
>> net/core/filter.c | 72 ++++++------
>> .../selftests/bpf/prog_tests/log_fixup.c | 2 +-
>> .../selftests/bpf/prog_tests/spin_lock.c | 8 +-
>> .../bpf/verifier/bounds_mix_sign_unsign.c | 110 ++++++++++--------
>> .../selftests/bpf/verifier/bpf_st_mem.c | 29 +++++
>> tools/testing/selftests/bpf/verifier/ctx.c | 11 --
>> tools/testing/selftests/bpf/verifier/unpriv.c | 23 ++++
>> 9 files changed, 249 insertions(+), 157 deletions(-)
>> create mode 100644 tools/testing/selftests/bpf/verifier/bpf_st_mem.c
>>
>> --
>> 2.39.0
>>
