Re: [PATCH bpf-next v8 3/9] bpf: Check potential private stack recursion for progs with async callback

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

On 11/1/24 12:55 PM, Alexei Starovoitov wrote:
> On Thu, Oct 31, 2024 at 8:12 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>> In previous patch, tracing progs are enabled for private stack since
>> recursion checking ensures there exists no nested same bpf prog run on
>> the same cpu.
>>
>> But it is still possible for nested bpf subprog run on the same cpu
>> if the same subprog is called in both main prog and async callback,
>> or in different async callbacks. For example,
>>    main_prog
>>     bpf_timer_set_callback(timer, timer_cb);
>>     call sub1
>>    sub1
>>     ...
>>    time_cb
>>     call sub1
>>
>> In the above case, nested subprog run for sub1 is possible with one in
>> process context and the other in softirq context. If this is the case,
>> the verifier will disable private stack for this bpf prog.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   kernel/bpf/verifier.c | 46 ++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index d3f4cbab97bc..596afd29f088 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -6070,7 +6070,8 @@ static int round_up_stack_depth(struct bpf_verifier_env *env, int stack_depth)
>>    */
>>   static int check_max_stack_depth_subprog(struct bpf_verifier_env *env, int idx,
>>                                           int *subtree_depth, int *depth_frame,
>> -                                        int priv_stack_supported)
>> +                                        int priv_stack_supported,
>> +                                        char *subprog_visited)
>>   {
>>          struct bpf_subprog_info *subprog = env->subprog_info;
>>          struct bpf_insn *insn = env->prog->insnsi;
>> @@ -6120,8 +6121,12 @@ static int check_max_stack_depth_subprog(struct bpf_verifier_env *env, int idx,
>>                                          idx, subprog_depth);
>>                                  return -EACCES;
>>                          }
>> -                       if (subprog_depth >= BPF_PRIV_STACK_MIN_SIZE)
>> +                       if (subprog_depth >= BPF_PRIV_STACK_MIN_SIZE) {
>>                                  subprog[idx].use_priv_stack = true;
>> +                               subprog_visited[idx] = 1;
>> +                       }
>> +               } else {
>> +                       subprog_visited[idx] = 1;
>>                  }
>>          }
>>   continue_func:
>> @@ -6222,19 +6227,42 @@ static int check_max_stack_depth_subprog(struct bpf_verifier_env *env, int idx,
>>   static int check_max_stack_depth(struct bpf_verifier_env *env)
>>   {
>>          struct bpf_subprog_info *si = env->subprog_info;
>> +       char *subprogs1 = NULL, *subprogs2 = NULL;
>>          int ret, subtree_depth = 0, depth_frame;
>> +       int orig_priv_stack_supported;
>>          int priv_stack_supported;
>>
>>          priv_stack_supported = bpf_enable_priv_stack(env);
>>          if (priv_stack_supported < 0)
>>                  return priv_stack_supported;
>>
>> +       orig_priv_stack_supported = priv_stack_supported;
>> +       if (orig_priv_stack_supported != NO_PRIV_STACK) {
>> +               subprogs1 = kvmalloc(env->subprog_cnt * 2, __GFP_ZERO);
> Just __GFP_ZERO ?!
>
> Overall the algo is messy. Pls think of a cleaner way of checking.
> Add two bool flags to bpf_subprog_info:
> visited_with_priv_stack
> visited_without_priv_stack

Actually this is what I thought initially as well with two bool flags
in bpf_subprog_info. Later on, I think since these two bool flags are
used ONLY in this function, probably not worthwhile to add them to
the head file.

But since you suggest this, I can go to two bool flag approach.

> and after walking all subrpogs add another loop over subprogs
> that checks for exclusivity of these flags?
>
> Probably other algos are possible.
>
>> +               if (!subprogs1)
>> +                       priv_stack_supported = NO_PRIV_STACK;
>> +               else
>> +                       subprogs2 = subprogs1 + env->subprog_cnt;
>> +       }
>> +
>>          for (int i = 0; i < env->subprog_cnt; i++) {
[...]