On Wed, Apr 23, 2025 at 12:33 AM Feng Yang <yangfeng59949@xxxxxxx> wrote: > > From: Feng Yang <yangfeng@xxxxxxxxxx> > > 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@xxxxxxxxxx> > Acked-by: Song Liu <song@xxxxxxxxxx> > --- 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@xxxxxxxxxxxxxx/) > - get_smp_processor_id also be retained(https://lore.kernel.org/all/CAADnVQ+WYLfoR1W6AsCJF6fNKEUgfxANXP01EQCJh1=99ZpoNw@xxxxxxxxxxxxxx/) yep, I saw the discussion, that's fine > > - Link to v3: https://lore.kernel.org/all/20250410070258.276759-1-yangfeng59949@xxxxxxx/ > > Changes in v3: > - Only modify patch description information. > - Link to v2: https://lore.kernel.org/all/20250408071151.229329-1-yangfeng59949@xxxxxxx/ > > Changes in v2: > - Only modify patch description information. > - Link to v1: https://lore.kernel.org/all/20250320032258.116156-1-yangfeng59949@xxxxxxx/ > --- > kernel/trace/bpf_trace.c | 72 ---------------------------------------- > 1 file changed, 72 deletions(-) > [...]
