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

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

On 7/2/25 12:48 AM, Arnd Bergmann wrote:
> On Tue, Jul 1, 2025, at 23:28, Yonghong Song wrote:
>> 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>
>>>> 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!
> I'm not sure if that actually helps on the old clang version, as far
> as I understood it in my initial analysis, the problem in the
>
> struct bpf_insn chk_and_sdiv[] = {
>                                  /* [R,W]x sdiv 0 -> 0
>                                   * LLONG_MIN sdiv -1 -> LLONG_MIN
>                                   * INT_MIN sdiv -1 -> INT_MIN
>                                   */
>                                  BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
> ...
> }
>
> construct is not the chk_and_sdiv[] array itself but the
> struct initializer in the BPF_MOV64_REG() macro that leads to
> having two copies of the struct on the stack and then copying
> between them. In gcc or clang-18+, these all get folded
> into a single object on the stack.

See https://lore.kernel.org/bpf/20250702053332.1991516-1-yonghong.song@linux.dev/.
The above 'struct bpf_insn chk_and_sdiv[] = { ... }' will be removed so
there will not be stack consumption any more for it. Instead, we use
the scratch space in bpf_verifier_env.

>
> (Disclaimer: I don't understand anything about how clang
> actually works internally, the above is only speculation on
> my side, based on the assembler output)
>
>        Arnd