Re: [PATCH bpf-next v9 02/10] bpf: Return false for bpf_prog_check_recur() default case

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

On 11/5/24 8:38 AM, Alexei Starovoitov wrote:
> On Tue, Nov 5, 2024 at 8:33 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>>
>>
>> On 11/5/24 7:50 AM, Alexei Starovoitov wrote:
>>> On Mon, Nov 4, 2024 at 10:02 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>>> I also don't understand the point of this patch 2.
>>>>> The patch 3 can still do:
>>>>>
>>>>> + switch (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 PRIV_STACK_ADAPTIVE;
>>>>> + default:
>>>>> +   break;
>>>>> + }
>>>>> +
>>>>> + if (!bpf_prog_check_recur(prog))
>>>>> +   return NO_PRIV_STACK;
>>>>>
>>>>> which would mean that iter, lsm, struct_ops will not be allowed
>>>>> to use priv stack.
>>>> One example is e.g. a TC prog. Since bpf_prog_check_recur(prog)
>>>> will return true (means supporting recursion), and private stack
>>>> does not really support TC prog, the logic will become more
>>>> complicated.
>>>>
>>>> I am totally okay with removing patch 2 and go back to my
>>>> previous approach to explicitly list prog types supporting
>>>> private stack.
>>> The point of reusing bpf_prog_check_recur() is that we don't
>>> need to duplicate the logic.
>>> We can still do something like:
>>> switch (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 PRIV_STACK_ADAPTIVE;
>>>    case BPF_PROG_TYPE_TRACING:
>>>    case BPF_PROG_TYPE_LSM:
>>>    case BPF_PROG_TYPE_STRUCT_OPS:
>>>       if (bpf_prog_check_recur())
>>>         return PRIV_STACK_ADAPTIVE;
>>>       /* fallthrough */
>>>     default:
>>>       return NO_PRIV_STACK;
>>> }
>> Right. Listing trampoline related prog types explicitly
>> and using bpf_prog_check_recur() will be safe.
>>
>> One thing is for BPF_PROG_TYPE_STRUCT_OPS, PRIV_STACK_ALWAYS
>> will be returned. I will make adjustment like
>>
>> switch (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 PRIV_STACK_ADAPTIVE;
>>    case BPF_PROG_TYPE_TRACING:
>>    case BPF_PROG_TYPE_LSM:
>>    case BPF_PROG_TYPE_STRUCT_OPS:
>>       if (bpf_prog_check_recur()) {
>>         if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
>>             return PRIV_STACK_ALWAYS;
> hmm. definitely not unconditionally.
> Only when explicitly requested in callback.
>
> Something like this:
>     case BPF_PROG_TYPE_TRACING:
>     case BPF_PROG_TYPE_LSM:
>        if (bpf_prog_check_recur())
>           return PRIV_STACK_ADAPTIVE;
>     case BPF_PROG_TYPE_STRUCT_OPS:
>        if (prog->aux->priv_stack_requested)
>           return PRIV_STACK_ALWAYS;
>     default:
>        return NO_PRIV_STACK;
>
> and then we also change bpf_prog_check_recur()
>   to return true when prog->aux->priv_stack_requested

This works too. I had another thinking about
    case BPF_PROG_TYPE_LSM:
       if (bpf_prog_check_recur())
          return PRIV_STACK_ADAPTIVE;
    case BPF_PROG_TYPE_STRUCT_OPS:
       if (bpf_prog_check_recur())
          return PRIV_STACK_ALWAYS;

Note that in bpf_prog_check_recur(), for struct_ops,
will return prog->aux->priv_stack_request.
But think it is too verbose so didn't propose.

So explicitly using prog->aux->priv_stack_requested
is more visible. Maybe we can even do

    case BPF_PROG_TYPE_TRACING:
    case BPF_PROG_TYPE_LSM:
    case BPF_PROG_TYPE_STRUCT_OPS:
       if (prog->aux->priv_stack_requested)
          return PRIV_STACK_ALWYAS;
       else if (bpf_prog_check_recur())
          return PRIV_STACK_ADAPTIVE;
       /* fallthrough */
    default:
       return NO_PRIV_STACK;