On Mon, Nov 4, 2024 at 5:35 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>
>
> On 11/4/24 5:21 PM, Alexei Starovoitov wrote:
> > On Mon, Nov 4, 2024 at 11:35 AM Yonghong Song <yonghong.song@xxxxxxxxx> 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.
