On Wed, Apr 16, 2025 at 2:56 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Thu, Apr 10, 2025 at 12:03 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. > > > > Signed-off-by: Feng Yang <yangfeng@xxxxxxxxxx> > > Acked-by: Song Liu <song@xxxxxxxxxx> > > --- > > 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(-) > > > > All this looks good, I checked that those functions indeed are allowed > in bpf_base_func_proto. The only (minor) differences are capabilities, > bpf_base_func_proto() correctly guards some of the helpers with > CAP_BPF and/or CAP_PERFMON checks, while bpf_tracing_func_proto() > doesn't seem to bother (which is either a bug or any tracing prog > implies CAP_BPF and CAP_PERFMON, I'm not sure, didn't check). > > But I think we can take it further and remove even more stuff from > bpf_tracing_func_proto and/or add more stuff into bpf_base_func_proto > (perhaps as a few patches in a series, so it's easier to review and > validate). > > Basically, except for a few custom implementations that depend on > tracing program type (like get_stack and others like that), if > something is OK to call from a tracing program it should be ok to call > from any program type. And as such it can (should?) be added to > bpf_base_func_proto, IMO. Is this true? Does it make sense? (See below.) > P.S. I'd name the patch/series as "bpf: streamline allowed helpers > between tracing and base sets" or something like that to make the > purpose clearer > > [...] > > > case BPF_FUNC_get_current_uid_gid: > > return &bpf_get_current_uid_gid_proto; > > case BPF_FUNC_get_current_comm: > > return &bpf_get_current_comm_proto; > > I'm surprised these two are not part of bpf_base_func_proto, tbh... > maybe let's move them there while we are cleaning all this up? Do these make sense in all BPF program types such that they belong in bpf_base_func_proto? For example, XDP programs don't have a current uid and gid, do they? > pw-bot: cr > > > - case BPF_FUNC_trace_printk: > > - return bpf_get_trace_printk_proto(); > > case BPF_FUNC_get_smp_processor_id: > > return &bpf_get_smp_processor_id_proto; > > this one should be cleaned up as well and > bpf_get_smp_processor_id_proto removed. All BPF programs either > disable CPU preemption or CPU migration, so bpf_base_func_proto's > implementation should work just fine (but please do it as a separate > patch) > > > - case BPF_FUNC_get_numa_node_id: > > - return &bpf_get_numa_node_id_proto; > > case BPF_FUNC_perf_event_read: > > return &bpf_perf_event_read_proto; > > [...] >
