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/4/24 5:55 PM, Alexei Starovoitov wrote:
> On Mon, Nov 4, 2024 at 5:35 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 11/4/24 5:21 PM, Alexei Starovoitov wrote:
>>> On Mon, Nov 4, 2024 at 11:35 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>> The bpf_prog_check_recur() funciton is currently used by trampoline
>>>> and tracing programs (also using trampoline) to check whether a
>>>> particular prog supports recursion checking or not. The default case
>>>> (non-trampoline progs) return true in the current implementation.
>>>>
>>>> Let us make the non-trampoline prog recursion check return false
>>>> instead. It does not impact any existing use cases and allows the
>>>> function to be used outside the trampoline context in the next patch.
>>> Does not impact ?! But it does.
>>> This patch removes recursion check from fentry progs.
>>> This cannot be right.
>> The original bpf_prog_check_recur() implementation:
>>
>> 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;
>>           }
>> }
>>
>> fentry prog is a TRACING prog, so it is covered. Did I miss anything?
> I see. This is way too subtle.
> You're correct that fentry is TYPE_TRACING,
> so it could have "worked" if it was used to build trampolines only.
>
> But this helper is called for other prog types:
>
>          case BPF_FUNC_task_storage_get:
>                  if (bpf_prog_check_recur(prog))
>                          return &bpf_task_storage_get_recur_proto;
>                  return &bpf_task_storage_get_proto;
>
> so it's still not correct, but for a different reason.

There are four uses for func bpf_prog_check_recur() in kernel based on 
cscope: 0 kernel/bpf/trampoline.c bpf_trampoline_enter 1053 if 
(bpf_prog_check_recur(prog)) 1 kernel/bpf/trampoline.c 
bpf_trampoline_exit 1068 if (bpf_prog_check_recur(prog)) 2 
kernel/trace/bpf_trace.c bpf_tracing_func_proto 1549 if 
(bpf_prog_check_recur(prog)) 3 kernel/trace/bpf_trace.c 
bpf_tracing_func_proto 1553 if (bpf_prog_check_recur(prog)) The 2nd and 
3rd ones are in bpf_trace.c. 1444 static const struct bpf_func_proto * 
1445 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct 
bpf_prog *prog) 1446 { 1447 switch (func_id) { ... 1548 case 
BPF_FUNC_task_storage_get: 1549 if (bpf_prog_check_recur(prog)) 1550 
return &bpf_task_storage_get_recur_proto; 1551 return 
&bpf_task_storage_get_proto; 1552 case BPF_FUNC_task_storage_delete: 
1553 if (bpf_prog_check_recur(prog)) 1554 return 
&bpf_task_storage_delete_recur_proto; 1555 return 
&bpf_task_storage_delete_proto; ... 1568 default: 1569 return 
bpf_base_func_proto(func_id, prog); 1570 } 1571 } They are used for 
tracing programs. So we should be safe here. But if you think that 
changing bpf_proc_check_recur() and calling function 
bpf_prog_check_recur() in bpf_enable_priv_stack() is too subtle, I can 
go back to my original approach which makes all supported prog types 
explicit in bpf_enable_priv_stack().