linux-kernel - Re: [PATCH bpf-next v4] bpf: streamline allowed helpers between tracing and base sets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4Bza6gK3dsrTosk6k3oZgtHesNDSrDd8sdeQ-GiS6oJixQg@mail.gmail.com>
Date: Wed, 23 Apr 2025 11:08:14 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Feng Yang <yangfeng59949@....com>
Cc: ast@...nel.org, daniel@...earbox.net, andrii@...nel.org, 
	martin.lau@...ux.dev, eddyz87@...il.com, song@...nel.org, 
	yonghong.song@...ux.dev, john.fastabend@...il.com, kpsingh@...nel.org, 
	sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org, 
	mattbobrowski@...gle.com, rostedt@...dmis.org, mhiramat@...nel.org, 
	mathieu.desnoyers@...icios.com, bpf@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v4] bpf: streamline allowed helpers between
 tracing and base sets

On Wed, Apr 23, 2025 at 12:33 AM Feng Yang <yangfeng59949@....com> wrote:
>
> From: Feng Yang <yangfeng@...inos.cn>
>
> Many conditional checks in switch-case are redundant
> with bpf_base_func_proto and should be removed.
>
> Regarding the permission checks bpf_base_func_proto:
> The permission checks in bpf_prog_load (as outlined below)
> ensure that the trace has both CAP_BPF and CAP_PERFMON capabilities,
> thus enabling the use of corresponding prototypes
> in bpf_base_func_proto without adverse effects.
> bpf_prog_load
>         ......
>         bpf_cap = bpf_token_capable(token, CAP_BPF);
>         ......
>         if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
>             type != BPF_PROG_TYPE_CGROUP_SKB &&
>             !bpf_cap)
>                 goto put_token;
>         ......
>         if (is_perfmon_prog_type(type) && !bpf_token_capable(token, CAP_PERFMON))
>                 goto put_token;
>         ......
>
> Signed-off-by: Feng Yang <yangfeng@...inos.cn>
> Acked-by: Song Liu <song@...nel.org>
> ---

LGTM, applied to bpf-next, thanks. See comments on remaining helpers below.

> Changes in v4:
> - Only modify patch description information.
> - At present, bpf_tracing_func_proto still has the following ID:
> - BPF_FUNC_get_current_uid_gid
> - BPF_FUNC_get_current_comm

I don't see why these two cannot be used in any program, after all, we
have bpf_get_current_task(), these are in the same family.

> - BPF_FUNC_get_smp_processor_id

Based on another thread, I think it's some filter programs that have
to use raw variant of it, right? All other should use non-raw
implementation. So I think the right next step would be to make sure
that bpf_base_func_proto returns non-raw implementation, and only
those few program types that are exceptions should use raw ones?

> - BPF_FUNC_perf_event_read

should be fine to use anywhere (and actually can be useful for
networking programs to measure its own packet processing overhead or
something like that). Checking implementation I don't see any
limitations, it's just PERF_EVENT_ARRAY map access

> - BPF_FUNC_probe_read
> - BPF_FUNC_probe_read_str

generic tracing helpers, should be OK to be used anywhere with
CAP_PERFMON capabilities

> - BPF_FUNC_current_task_under_cgroup

same as above current_comm, if there is CGROUP_ARRAY, this should be
fine (though I don't know, there might be cgroup-specific
restrictions, not sure)

> - BPF_FUNC_send_signal
> - BPF_FUNC_send_signal_thread

fine to do from NMI, so should be fine to do anywhere (with
CAP_PERFMON, presumably)

> - BPF_FUNC_get_task_stack

seems fine (again, if it works under NMI and doesn't use any
context-dependent things, should be fine for any program type)

> - BPF_FUNC_copy_from_user
> - BPF_FUNC_copy_from_user_task

same as probe_read/probe_read_str (but only for sleepable)

> - BPF_FUNC_task_storage_get
> - BPF_FUNC_task_storage_delete

this is designed to work anywhere, so yeah, why not?

> - BPF_FUNC_get_func_ip

nope, very context dependent, definitely not generic (and just doesn't
make sense for most program types)

> - BPF_FUNC_get_branch_snapshot

NMI-enabled and not context-dependent, good to be used anywhere

> - BPF_FUNC_find_vma

non-sleepable, but other than that doesn't really make any assumptions
about program type, should be fine everywhere (NMI-safe, I believe?)

> - BPF_FUNC_probe_write_user

it's just like probe_read_user, CAP_PERFMON, so we can enable it
anywhere for completeness, but I'm not sure if that is a good idea...

> - I'm not sure which ones can be used by all programs, as Zvi Effron said(https://lore.kernel.org/all/CAC1LvL2SOKojrXPx92J46fFEi3T9TAWb3mC1XKtYzwU=pzTEbQ@mail.gmail.com/)
> - get_smp_processor_id also be retained(https://lore.kernel.org/all/CAADnVQ+WYLfoR1W6AsCJF6fNKEUgfxANXP01EQCJh1=99ZpoNw@mail.gmail.com/)

yep, I saw the discussion, that's fine

>
> - Link to v3: https://lore.kernel.org/all/20250410070258.276759-1-yangfeng59949@163.com/
>
> Changes in v3:
> - Only modify patch description information.
> - Link to v2: https://lore.kernel.org/all/20250408071151.229329-1-yangfeng59949@163.com/
>
> Changes in v2:
> - Only modify patch description information.
> - Link to v1: https://lore.kernel.org/all/20250320032258.116156-1-yangfeng59949@163.com/
> ---
>  kernel/trace/bpf_trace.c | 72 ----------------------------------------
>  1 file changed, 72 deletions(-)
>

[...]