On Wed, Apr 16, 2025 at 3:44 PM Zvi Effron <[email protected]> wrote: > > On Wed, Apr 16, 2025 at 2:56 PM Andrii Nakryiko > <[email protected]> wrote: > > > > On Thu, Apr 10, 2025 at 12:03 AM Feng Yang <[email protected]> wrote: > > > > > > From: Feng Yang <[email protected]> > > > > > > Many conditional checks in switch-case are redundant > > > with bpf_base_func_proto and should be removed. > > > > > > Signed-off-by: Feng Yang <[email protected]> > > > Acked-by: Song Liu <[email protected]> > > > --- > > > Changes in v3: > > > - Only modify patch description information. > > > - Link to v2: > > > https://lore.kernel.org/all/[email protected]/ > > > > > > Changes in v2: > > > - Only modify patch description information. > > > - Link to v1: > > > https://lore.kernel.org/all/[email protected]/ > > > --- > > > 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?
everything in the kernel, whether NMI handler, hardirq, softirq, or whatnot runs with *some* current task. So in that sense there is always pid/tgid and uid/gid. It might not be very relevant for XDP programs, but it's there, and so if we allow to get current pid/tgid, why not allow the current comm and uid/gid? > > > 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; > > > > [...] > >
