Re: [PATCH bpf-next v3] bpf: Remove redundant checks

[Zvi Effron]

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;
>
> [...]
>