Re: [PATCH] bpf: turn off sanitizer in do_misc_fixups for old clang

[Yonghong Song <yonghong.song@linux.dev>]

On 7/1/25 1:45 PM, Andrii Nakryiko wrote:
> On Tue, Jul 1, 2025 at 1:03 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>>
>> On 6/23/25 2:32 PM, Alexei Starovoitov wrote:
>>> On Fri, Jun 20, 2025 at 4:38 AM Arnd Bergmann <arnd@kernel.org> wrote:
>>>> From: Arnd Bergmann <arnd@arndb.de>
>>>>
>>>> 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@arndb.de>
>>>> ---
>>>>    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?

Make sense. This is much better. Thanks!