Re: [PATCH bpf-next v9 03/10] bpf: Allow private stack to have each subprog having stack size of 512 bytes

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

On 11/4/24 6:47 PM, Alexei Starovoitov wrote:
> On Mon, Nov 4, 2024 at 11:35 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>> @@ -6070,11 +6105,23 @@ static int check_max_stack_depth_subprog(struct bpf_verifier_env *env, int idx,
>>                          depth);
>>                  return -EACCES;
>>          }
>> -       depth += round_up_stack_depth(env, subprog[idx].stack_depth);
>> +       subprog_depth = round_up_stack_depth(env, subprog[idx].stack_depth);
>> +       depth += subprog_depth;
>>          if (depth > MAX_BPF_STACK && !*subtree_depth) {
>>                  *subtree_depth = depth;
>>                  *depth_frame = frame + 1;
>>          }
>> +       if (priv_stack_supported != NO_PRIV_STACK) {
>> +               if (!subprog[idx].use_priv_stack) {
>> +                       if (subprog_depth > MAX_BPF_STACK) {
>> +                               verbose(env, "stack size of subprog %d is %d. Too large\n",
>> +                                       idx, subprog_depth);
>> +                               return -EACCES;
>> +                       }
>> +                       if (subprog_depth >= BPF_PRIV_STACK_MIN_SIZE)
>> +                               subprog[idx].use_priv_stack = true;
>> +               }
>> +       }
> Hold on. If I'm reading this correctly this adaptive priv stack
> concept will make some subprogs with stack >= 64 to use priv_stack
> while other subprogs will still use normal stack?
> Same for the main prog. It may or may not use priv stack ?
>
> I guess this is ok-ish, but needs to be clearly explained in comments
> and commit log.

Will do.

> My first reaction to such adaptive concept was negative, since
> such "random" mix of priv stack in some subprogs makes
> the whole thing pretty hard to reason about it,
> but I guess it's valid to use normal stack when stack usage
> is small. No need to penalize every subprog.
>
> I wonder what others think about it.

Ya, other opinions are very welcome!

>
> Also it would be cleaner to rewrite above as:
> if (subprog_depth > MAX_BPF_STACK) {
>     verbose();
>     return -EACCESS;
> }
> if (priv_stack_supported == PRIV_STACK_ADAPTIVE &&
>      subprog_depth >= BPF_PRIV_STACK_MIN_SIZE)
>     subprog[idx].use_priv_stack = true;
>
> less indent and easier to read.

Okay, will do with less indent.