Re: [PATCH bpf-next 01/18] bpf: Support stack arguments for bpf functions

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

On 4/24/26 10:09 PM, Yonghong Song wrote:
>
>
> On 4/24/26 11:13 AM, bot+bpf-ci@kernel.org wrote:
>>> diff --git a/include/linux/bpf_verifier.h 
>>> b/include/linux/bpf_verifier.h
>>> index d5b4303315dd..2cc349d7fc17 100644
>>> --- a/include/linux/bpf_verifier.h
>>> +++ b/include/linux/bpf_verifier.h
>> [ ... ]
>>
>>> @@ -739,10 +759,13 @@ struct bpf_subprog_info {
>>>       bool keep_fastcall_stack: 1;
>>>       bool changes_pkt_data: 1;
>>>       bool might_sleep: 1;
>>> -    u8 arg_cnt:3;
>>> +    u8 arg_cnt:4;
>>>
>>>       enum priv_stack_mode priv_stack_mode;
>>> -    struct bpf_subprog_arg_info args[MAX_BPF_FUNC_REG_ARGS];
>>> +    struct bpf_subprog_arg_info args[MAX_BPF_FUNC_ARGS];
>>> +    u16 incoming_stack_arg_depth;
>>> +    u16 stack_arg_depth; /* incoming + max outgoing */
>>> +    u16 max_out_stack_arg_depth;
>>>   };
>> Alexei Starovoitov raised a concern in the v6 review about duplicated
>> fields. The incoming_stack_arg_depth, stack_arg_depth, and
>> max_out_stack_arg_depth fields are present in both bpf_subprog_info and
>> bpf_prog_aux structures. His comment was:
>>
>>    "but you already have them in prog_aux?! another copy in
>>    bpf_subprog_info?! Remove one of them. JIT only need one set."
>>
>> Looking at include/linux/bpf.h, these fields appear in bpf_prog_aux:
>>
>>    struct bpf_prog_aux {
>>        ...
>>        u16 incoming_stack_arg_depth;
>>        u16 stack_arg_depth;
>>        ...
>>    };
>>
>> Does the verifier actually need both copies? The concern was that having
>> duplicate state creates maintenance burden.
>
> Before jit_subprogs(), bpf_subprog_info stores the information for 
> each subprog.
> At this point, the subprog has not been allocated and only main prog 
> is available.
> Therefore, it is not possible to copy bpf_subprog_info to each subprog.
>
> So I think it might be necessary to have both fields in bpf_prog_aux and
> bpf_subprog_info.
>
> This version made some changes so max_out_stack_arg_depth is not needed
> any more. Will remove it in the next revision.

I did some futher checking and found that max_out_stack_arg_depth is still
needed. Otherwise, they could silently corrupting stack. For example,
for
     bar(int a) {
       ...
       foo(a1, a2, a3, a4, a5, a6);
       ...
     }

let us say before foo(), we have
     *(u64 *)(r11 - 8) = ...
     *(u64 *)(r11 - 16) = ...
     call foo() ...

In JIT, the outgoing stack will be 8 bytes (for *(u64 *)(r11 - 8) = ...).
But due to '*(u64 *)(r11 - 16) = ...', it may have an outgoing stack writing
with offset -16, which may cause an issue since other applicaiton, e.g.,
nmi/irq etc. So in such ases, we should reject in verifier.

>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index ff6ff1c27517..bcf81692a22b 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c 

[...]