Re: [PATCH bpf-next] bpf: Allow some trace helpers for all prog types

[Andrii Nakryiko]

On Fri, Apr 25, 2025 at 1:02 AM Feng Yang <yangfeng59949@xxxxxxx> wrote:
>
> From: Feng Yang <yangfeng@xxxxxxxxxx>
>
> if it works under NMI and doesn't use any context-dependent things,
> should be fine for any program type. The detailed discussion is in [1].
>
> [1] https://lore.kernel.org/all/CAEf4Bza6gK3dsrTosk6k3oZgtHesNDSrDd8sdeQ-GiS6oJixQg@xxxxxxxxxxxxxx/
>
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> Signed-off-by: Feng Yang <yangfeng@xxxxxxxxxx>
> ---
>  kernel/bpf/cgroup.c      |  6 -----
>  kernel/bpf/helpers.c     | 50 +++++++++++++++++++++++++++++++++++++
>  kernel/trace/bpf_trace.c | 53 +++++-----------------------------------
>  net/core/filter.c        |  2 --
>  4 files changed, 56 insertions(+), 55 deletions(-)
>

Some tests in CI are failing, please check and fix. Also see below
about compat probe_read APIs.


> @@ -2037,6 +2052,16 @@ bpf_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return &bpf_get_current_task_proto;
>         case BPF_FUNC_get_current_task_btf:
>                 return &bpf_get_current_task_btf_proto;
> +       case BPF_FUNC_get_current_comm:
> +               return &bpf_get_current_comm_proto;
> +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> +       case BPF_FUNC_probe_read:
> +               return security_locked_down(LOCKDOWN_BPF_READ_KERNEL) < 0 ?
> +                      NULL : &bpf_probe_read_compat_proto;
> +       case BPF_FUNC_probe_read_str:
> +               return security_locked_down(LOCKDOWN_BPF_READ_KERNEL) < 0 ?
> +                      NULL : &bpf_probe_read_compat_str_proto;
> +#endif

No, let's not expose compat probe read APIs to more program types,
these should eventually go away

pw-bot: cr

>         case BPF_FUNC_probe_read_user:
>                 return &bpf_probe_read_user_proto;
>         case BPF_FUNC_probe_read_kernel:
> @@ -2057,6 +2082,31 @@ bpf_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return bpf_get_trace_vprintk_proto();
>         case BPF_FUNC_perf_event_read_value:
>                 return bpf_get_perf_event_read_value_proto();
> +       case BPF_FUNC_perf_event_read:
> +               return &bpf_perf_event_read_proto;
> +       case BPF_FUNC_send_signal:
> +               return &bpf_send_signal_proto;
> +       case BPF_FUNC_send_signal_thread:
> +               return &bpf_send_signal_thread_proto;
> +       case BPF_FUNC_get_task_stack:
> +               return prog->sleepable ? &bpf_get_task_stack_sleepable_proto
> +                                      : &bpf_get_task_stack_proto;
> +       case BPF_FUNC_copy_from_user:
> +               return prog->sleepable ? &bpf_copy_from_user_proto : NULL;
> +       case BPF_FUNC_copy_from_user_task:
> +               return prog->sleepable ? &bpf_copy_from_user_task_proto : NULL;

I'd put these two next to probe_read APIs above

> +       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;
> +       case BPF_FUNC_task_storage_delete:
> +               if (bpf_prog_check_recur(prog))
> +                       return &bpf_task_storage_delete_recur_proto;
> +               return &bpf_task_storage_delete_proto;
> +       case BPF_FUNC_get_branch_snapshot:
> +               return &bpf_get_branch_snapshot_proto;
> +       case BPF_FUNC_find_vma:
> +               return &bpf_find_vma_proto;
>         default:
>                 return NULL;
>         }

[...]