[PATCH v2] bpf: make function do_misc_fixups as noinline_for_stack

[PATCH v2] bpf: make function do_misc_fixups as noinline_for_stack

[flyingpenghao]

From: Peng Hao <flyingpeng@tencent.com>

When KASAN is enabled and built with clang:
kernel/bpf/verifier.c:21486:5: error: stack frame size (2264) exceeds limit (2048) in 'bpf_check' [-Werror,-Wframe-larger-than]
int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size)
    ^

By tracing the call chain, we found that do_misc_fixups consumed a lot
of stack space. mark it as noinline_for_stack to prevent it from spreading
to bpf_check's stack size.

Signed-off-by: Peng Hao <flyingpeng@tencent.com>
---
 kernel/bpf/verifier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 48f3a9acdef3..ba1bf742a33d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19791,7 +19791,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 noinline_for_stack int do_misc_fixups(struct bpf_verifier_env *env)
 {
 	struct bpf_prog *prog = env->prog;
 	enum bpf_attach_type eatype = prog->expected_attach_type;
-- 
2.27.0

Re: [PATCH v2] bpf: make function do_misc_fixups as noinline_for_stack

[Alexei Starovoitov]

On Wed, Jul 10, 2024 at 10:45 PM <flyingpenghao@gmail.com> wrote:
>
>
> By tracing the call chain, we found that do_misc_fixups consumed a lot
> of stack space. mark it as noinline_for_stack to prevent it from spreading
> to bpf_check's stack size.

...
> -static int do_misc_fixups(struct bpf_verifier_env *env)
> +static noinline_for_stack int do_misc_fixups(struct bpf_verifier_env *env)

Now we're getting somewhere, but this is not a fix.
It may shut up the warn, but it will only increase the total stack usage.
Looking at C code do_misc_fixups() needs ~200 bytes worth of stack
space for insn_buf[16] and spill/fill.
That's far from the artificial 2k limit.

Please figure out what exact variable is causing kasan to consume
so much stack. You may need to analyze compiler internals and
do more homework.
What is before/after stack usage ? with and without kasan?
With gcc try
+CFLAGS_verifier.o += -fstack-usage

I see:
sort -k2 -n kernel/bpf/verifier.su |tail -10
../kernel/bpf/verifier.c:13087:12:adjust_ptr_min_max_vals    240
dynamic,bounded
../kernel/bpf/verifier.c:20804:12:do_check_common    248    dynamic,bounded
../kernel/bpf/verifier.c:19151:12:convert_ctx_accesses    256    static
../kernel/bpf/verifier.c:7450:12:check_mem_reg    256    static
../kernel/bpf/verifier.c:7482:12:check_kfunc_mem_size_reg    256    static
../kernel/bpf/verifier.c:10268:12:check_helper_call.isra    272
dynamic,bounded
../kernel/bpf/verifier.c:21562:5:bpf_check    296    dynamic,bounded
../kernel/bpf/verifier.c:19860:12:do_misc_fixups    320    static
../kernel/bpf/verifier.c:13991:12:adjust_reg_min_max_vals    392    static
../kernel/bpf/verifier.c:12280:12:check_kfunc_call.isra    408
dynamic,bounded

do_misc_fixups() is not the smallest, but not that large either.

Do in-depth analysis instead of silencing the warn.

pw-bot: cr

Re: [PATCH v2] bpf: make function do_misc_fixups as noinline_for_stack

[Hao Peng]

On Sat, Jul 13, 2024 at 12:43 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Jul 10, 2024 at 10:45 PM <flyingpenghao@gmail.com> wrote:
> >
> >
> > By tracing the call chain, we found that do_misc_fixups consumed a lot
> > of stack space. mark it as noinline_for_stack to prevent it from spreading
> > to bpf_check's stack size.
>
> ...
> > -static int do_misc_fixups(struct bpf_verifier_env *env)
> > +static noinline_for_stack int do_misc_fixups(struct bpf_verifier_env *env)
>
> Now we're getting somewhere, but this is not a fix.
> It may shut up the warn, but it will only increase the total stack usage.
> Looking at C code do_misc_fixups() needs ~200 bytes worth of stack
> space for insn_buf[16] and spill/fill.
> That's far from the artificial 2k limit.
>
> Please figure out what exact variable is causing kasan to consume
> so much stack. You may need to analyze compiler internals and
> do more homework.
> What is before/after stack usage ? with and without kasan?
> With gcc try
> +CFLAGS_verifier.o += -fstack-usage
>
> I see:
> sort -k2 -n kernel/bpf/verifier.su |tail -10
> ../kernel/bpf/verifier.c:13087:12:adjust_ptr_min_max_vals    240
> dynamic,bounded
> ../kernel/bpf/verifier.c:20804:12:do_check_common    248    dynamic,bounded
> ../kernel/bpf/verifier.c:19151:12:convert_ctx_accesses    256    static
> ../kernel/bpf/verifier.c:7450:12:check_mem_reg    256    static
> ../kernel/bpf/verifier.c:7482:12:check_kfunc_mem_size_reg    256    static
> ../kernel/bpf/verifier.c:10268:12:check_helper_call.isra    272
> dynamic,bounded
> ../kernel/bpf/verifier.c:21562:5:bpf_check    296    dynamic,bounded
> ../kernel/bpf/verifier.c:19860:12:do_misc_fixups    320    static
> ../kernel/bpf/verifier.c:13991:12:adjust_reg_min_max_vals    392    static
> ../kernel/bpf/verifier.c:12280:12:check_kfunc_call.isra    408
> dynamic,bounded
>
> do_misc_fixups() is not the smallest, but not that large either.
>
If I use gcc, I get the same result as you, but if I use llvm to build
the kernel, the result is like this：
# sort -k2 -n kernel/bpf/verifier.su | tail -10
kernel/bpf/verifier.c:14026:adjust_reg_min_max_vals     440     static
kernel/bpf/verifier.c:7432:check_mem_reg        440     static
kernel/bpf/verifier.c:15955:check_cfg   472     static
kernel/bpf/verifier.c:7464:check_kfunc_mem_size_reg     472     static
kernel/bpf/verifier.c:15104:check_cond_jmp_op   504     static
kernel/bpf/verifier.c:4166:__mark_chain_precision       504     static
kernel/bpf/verifier.c:10239:check_helper_call   536     static
kernel/bpf/verifier.c:17744:do_check    792     static
kernel/bpf/verifier.c:12248:check_kfunc_call    984     static
kernel/bpf/verifier.c:21486:bpf_check   2456    static

Obviously, do_misc_fixups is automatically inlined into bpf_check.
So adding noinline_for_stack to the do_misc_fixups function is a solution.

Thanks.

> Do in-depth analysis instead of silencing the warn.
>
> pw-bot: cr

Re: [PATCH v2] bpf: make function do_misc_fixups as noinline_for_stack

[Yonghong Song]

On 7/24/24 11:18 PM, Hao Peng wrote:
> On Sat, Jul 13, 2024 at 12:43 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Wed, Jul 10, 2024 at 10:45 PM <flyingpenghao@gmail.com> wrote:
>>>
>>> By tracing the call chain, we found that do_misc_fixups consumed a lot
>>> of stack space. mark it as noinline_for_stack to prevent it from spreading
>>> to bpf_check's stack size.
>> ...
>>> -static int do_misc_fixups(struct bpf_verifier_env *env)
>>> +static noinline_for_stack int do_misc_fixups(struct bpf_verifier_env *env)
>> Now we're getting somewhere, but this is not a fix.
>> It may shut up the warn, but it will only increase the total stack usage.
>> Looking at C code do_misc_fixups() needs ~200 bytes worth of stack
>> space for insn_buf[16] and spill/fill.
>> That's far from the artificial 2k limit.
>>
>> Please figure out what exact variable is causing kasan to consume
>> so much stack. You may need to analyze compiler internals and
>> do more homework.
>> What is before/after stack usage ? with and without kasan?
>> With gcc try
>> +CFLAGS_verifier.o += -fstack-usage
>>
>> I see:
>> sort -k2 -n kernel/bpf/verifier.su |tail -10
>> ../kernel/bpf/verifier.c:13087:12:adjust_ptr_min_max_vals    240
>> dynamic,bounded
>> ../kernel/bpf/verifier.c:20804:12:do_check_common    248    dynamic,bounded
>> ../kernel/bpf/verifier.c:19151:12:convert_ctx_accesses    256    static
>> ../kernel/bpf/verifier.c:7450:12:check_mem_reg    256    static
>> ../kernel/bpf/verifier.c:7482:12:check_kfunc_mem_size_reg    256    static
>> ../kernel/bpf/verifier.c:10268:12:check_helper_call.isra    272
>> dynamic,bounded
>> ../kernel/bpf/verifier.c:21562:5:bpf_check    296    dynamic,bounded
>> ../kernel/bpf/verifier.c:19860:12:do_misc_fixups    320    static
>> ../kernel/bpf/verifier.c:13991:12:adjust_reg_min_max_vals    392    static
>> ../kernel/bpf/verifier.c:12280:12:check_kfunc_call.isra    408
>> dynamic,bounded
>>
>> do_misc_fixups() is not the smallest, but not that large either.
>>
> If I use gcc, I get the same result as you, but if I use llvm to build
> the kernel, the result is like this：
> # sort -k2 -n kernel/bpf/verifier.su | tail -10
> kernel/bpf/verifier.c:14026:adjust_reg_min_max_vals     440     static
> kernel/bpf/verifier.c:7432:check_mem_reg        440     static
> kernel/bpf/verifier.c:15955:check_cfg   472     static
> kernel/bpf/verifier.c:7464:check_kfunc_mem_size_reg     472     static
> kernel/bpf/verifier.c:15104:check_cond_jmp_op   504     static
> kernel/bpf/verifier.c:4166:__mark_chain_precision       504     static
> kernel/bpf/verifier.c:10239:check_helper_call   536     static
> kernel/bpf/verifier.c:17744:do_check    792     static
> kernel/bpf/verifier.c:12248:check_kfunc_call    984     static
> kernel/bpf/verifier.c:21486:bpf_check   2456    static
>
> Obviously, do_misc_fixups is automatically inlined into bpf_check.
> So adding noinline_for_stack to the do_misc_fixups function is a solution.

Looks like you are building your own kernel with KASAN.
You can change config CONFIG_FRAME_WARN value. In your config file you
have CONFIG_FRAME_WARN=2048. You can change it to
CONFIG_FRAME_WARN=4096 which should fix the issue.

>
> Thanks.
>
>> Do in-depth analysis instead of silencing the warn.
>>
>> pw-bot: cr