On Tue, Jul 1, 2025 at 1:03 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>
>
>
> On 6/23/25 2:32 PM, Alexei Starovoitov wrote:
> > On Fri, Jun 20, 2025 at 4:38 AM Arnd Bergmann <arnd@xxxxxxxxxx> wrote:
> >> From: Arnd Bergmann <arnd@xxxxxxxx>
> >>
> >> clang versions before version 18 manage to badly optimize the bpf
> >> verifier, with lots of variable spills leading to excessive stack
> >> usage in addition to likely rather slow code:
> >>
> >> kernel/bpf/verifier.c:23936:5: error: stack frame size (2096) exceeds limit (1280) in 'bpf_check' [-Werror,-Wframe-larger-than]
> >> kernel/bpf/verifier.c:21563:12: error: stack frame size (1984) exceeds limit (1280) in 'do_misc_fixups' [-Werror,-Wframe-larger-than]
> >>
> >> Turn off the sanitizer in the two functions that suffer the most from
> >> this when using one of the affected clang version.
> >>
> >> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> >> ---
> >> kernel/bpf/verifier.c | 11 +++++++++--
> >> 1 file changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 2fa797a6d6a2..7724c7a56d79 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -19810,7 +19810,14 @@ static int do_check_insn(struct bpf_verifier_env *env, bool *do_print_state)
> >> return 0;
> >> }
> >>
> >> -static int do_check(struct bpf_verifier_env *env)
> >> +#if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 180100
> >> +/* old clang versions cause excessive stack usage here */
> >> +#define __workaround_kasan __disable_sanitizer_instrumentation
> >> +#else
> >> +#define __workaround_kasan
> >> +#endif
> >> +
> >> +static __workaround_kasan int do_check(struct bpf_verifier_env *env)
> > This looks too hacky for a workaround.
> > Let's figure out what's causing such excessive stack usage and fix it.
> > We did some of this work in
> > commit 6f606ffd6dd7 ("bpf: Move insn_buf[16] to bpf_verifier_env")
> > and similar.
> > Looks like it wasn't enough or more stack usage crept in since then.
> >
> > Also make sure you're using the latest bpf-next.
> > A bunch of code was moved out of do_check().
> > So I bet the current bpf-next/master doesn't have a problem
> > with this particular function.
> > In my kasan build do_check() is now fully inlined.
> > do_check_common() is not and it's using 512 bytes of stack.
> >
> >> {
> >> bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);
> >> struct bpf_verifier_state *state = env->cur_state;
> >> @@ -21817,7 +21824,7 @@ static int add_hidden_subprog(struct bpf_verifier_env *env, struct bpf_insn *pat
> >> /* Do various post-verification rewrites in a single program pass.
> >> * These rewrites simplify JIT and interpreter implementations.
> >> */
> >> -static int do_misc_fixups(struct bpf_verifier_env *env)
> >> +static __workaround_kasan int do_misc_fixups(struct bpf_verifier_env *env)
> > This one is using 832 byte of stack with kasan.
> > Which is indeed high.
> > Big chunk seems to be coming from chk_and_sdiv[] and chk_and_smod[].
> >
> > Yonghong,
> > looks like you contributed that piece of code.
> > Pls see how to reduce stack size here.
> > Daniel used this pattern in earlier commits. Looks like
> > we took it too far.
>
> With llvm17, I got the following error:
>
> /home/yhs/work/bpf-next/kernel/bpf/verifier.c:24491:5: error: stack frame size (2552) exceeds limit (1280) in 'bpf_check' [-
> Werror,-Wframe-larger-than]
> 24491 | int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size)
> | ^
> /home/yhs/work/bpf-next/kernel/bpf/verifier.c:19921:12: error: stack frame size (1368) exceeds limit (1280) in 'do_check' [-
> Werror,-Wframe-larger-than]
> 19921 | static int do_check(struct bpf_verifier_env *env)
> | ^
> 2 errors generated.
>
> I checked IR and found the following memory allocations which may contribute
> excessive stack usage:
>
> attr.coerce1, i32 noundef %uattr_size) local_unnamed_addr #0 align 16 !dbg !19800 {
> entry:
> %zext_patch.i = alloca [2 x %struct.bpf_insn], align 16, !DIAssignID !19854
> %rnd_hi32_patch.i = alloca [4 x %struct.bpf_insn], align 16, !DIAssignID !19855
> %cnt.i = alloca i32, align 4, !DIAssignID !19856
> %patch.i766 = alloca [3 x %struct.bpf_insn], align 16, !DIAssignID !19857
> %chk_and_sdiv.i = alloca [1 x %struct.bpf_insn], align 4, !DIAssignID !19858
> %chk_and_smod.i = alloca [1 x %struct.bpf_insn], align 4, !DIAssignID !19859
> %chk_and_div.i = alloca [4 x %struct.bpf_insn], align 16, !DIAssignID !19860
> %chk_and_mod.i = alloca [4 x %struct.bpf_insn], align 16, !DIAssignID !19861
> %chk_and_sdiv343.i = alloca [8 x %struct.bpf_insn], align 16, !DIAssignID !19862
> %chk_and_smod472.i = alloca [9 x %struct.bpf_insn], align 16, !DIAssignID !19863
> %desc.i = alloca %struct.bpf_jit_poke_descriptor, align 8, !DIAssignID !19864
> %target_size.i = alloca i32, align 4, !DIAssignID !19865
> %patch.i = alloca [2 x %struct.bpf_insn], align 16, !DIAssignID !19866
> %patch355.i = alloca [2 x %struct.bpf_insn], align 16, !DIAssignID !19867
> %ja.i = alloca %struct.bpf_insn, align 8, !DIAssignID !19868
> %ret_insn.i.i = alloca [8 x i32], align 16, !DIAssignID !19869
> %ret_prog.i.i = alloca [8 x i32], align 16, !DIAssignID !19870
> %fd.i = alloca i32, align 4, !DIAssignID !19871
> %log_true_size = alloca i32, align 4, !DIAssignID !19872
> ...
>
> So yes, chk_and_{div,mod,sdiv,smod} consumes quite some stack and
> can be coverted to runtime allocation but that is not enough for 1280
> stack limit, we need to do more conversion from stack to memory
> allocation. Will try to have uniform way to convert
> 'alloca [<num> x %struct.bpf_insn]' to runtime allocation.
>
Do we need to go all the way to dynamic allocation? See env->insns_buf
(which some parts of this function are already using for constructing
instruction patch), let's just converge on that? It pre-allocates
space for 32 instructions, should be sufficient for all the use cases,
no?
