Re: [PATCH bpf-next v6 1/9] bpf: Allow each subprog having stack size of 512 bytes

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

On 10/22/24 3:59 PM, Alexei Starovoitov wrote:
> On Tue, Oct 22, 2024 at 3:41 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 10/22/24 2:57 PM, Alexei Starovoitov wrote:
>>> On Tue, Oct 22, 2024 at 2:43 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>> To handle a subprog may be used in more than one
>>>> subtree (subprog 0 tree or async tree), I need to
>>>> add a 'visited' field to bpf_subprog_info.
>>>> I think this should work.
>>> This is getting quite complicated.
>>>
>>> But looks like we have even bigger problem:
>>>
>>> SEC("lsm/...")
>>> int BPF_PROG(...)
>>> {
>>>     volatile char buf[..];
>>>     buf[..] =
>>> }
>> If I understand correctly, lsm/... corresponds to BPF_PROG_TYPE_LSM prog type.
>> The current implementation only supports the following plus struct_ops programs.
>>
>> +       switch (env->prog->type) {
>> +       case BPF_PROG_TYPE_KPROBE:
>> +       case BPF_PROG_TYPE_TRACEPOINT:
>> +       case BPF_PROG_TYPE_PERF_EVENT:
>> +       case BPF_PROG_TYPE_RAW_TRACEPOINT:
>> +               return true;
>> +       case BPF_PROG_TYPE_TRACING:
>> +               if (env->prog->expected_attach_type != BPF_TRACE_ITER)
>> +                       return true;
>> +               fallthrough;
>> +       default:
>> +               return false;
>> +       }
>>
>> I do agree that lsm programs will have issues if using private stack
>> since preemptible is possible and we don't have recursion check for
>> them (which is right in order to provide correct functionality).
> static inline bool bpf_prog_check_recur(const struct bpf_prog *prog)
> {
>          switch (resolve_prog_type(prog)) {
>          case BPF_PROG_TYPE_TRACING:
>                  return prog->expected_attach_type != BPF_TRACE_ITER;
>          case BPF_PROG_TYPE_STRUCT_OPS:
>          case BPF_PROG_TYPE_LSM:
>                  return false;
>          default:
>                  return true;
>          }
> }
>
> LSM prog is an example. The same issue is with struct_ops progs.
> But struct_ops sched-ext progs is main motivation for adding
> priv stack.
>
> sched-ext will signal to bpf that it needs priv stack and
> we would have to add "recursion no more than 1" check
> and there is a chance (like above LSM prog demonstrates)
> that struct_ops will be hitting this recursion check
> and the prog will not be run.
> The miss count will increment, of course, but the whole
> priv stack feature for struct_ops becomes unreliable.
> Hence the patches become questionable.
> Why add a feature when the main user will struggle to use it.

Indeed, this is a known issue we kind of already aware of.
The recursion check (regardless it is one or four) may cause
prog no run if actual recursion level is beyond what recursion
check is doing.

I guess we indeed need to go back to drawing board again,
starting from struct_ops which is the main motivation of this
idea.