[bpf-next,v3] bpf: Remove redundant checks

Commit Message

Feng Yang April 10, 2025, 7:02 a.m. UTC

From: Feng Yang <[e-mail]>

Many conditional checks in switch-case are redundant
with bpf_base_func_proto and should be removed.

Signed-off-by: Feng Yang <[e-mail]>
Acked-by: Song Liu <[e-mail]>
---
Changes in v3:
- Only modify patch description information.
- Link to v2: https://lore.kernel.org/all/[e-mail]/

Changes in v2:
- Only modify patch description information.
- Link to v1: https://lore.kernel.org/all/[e-mail]/
---
 kernel/trace/bpf_trace.c | 72 ----------------------------------------
 1 file changed, 72 deletions(-)

Comments

Andrii Nakryiko April 16, 2025, 9:55 p.m. UTC | #1

On Thu, Apr 10, 2025 at 12:03 AM Feng Yang <[e-mail]> wrote:
>
> From: Feng Yang <[e-mail]>
>
> Many conditional checks in switch-case are redundant
> with bpf_base_func_proto and should be removed.
>
> Signed-off-by: Feng Yang <[e-mail]>
> Acked-by: Song Liu <[e-mail]>
> ---
> Changes in v3:
> - Only modify patch description information.
> - Link to v2: https://lore.kernel.org/all/[e-mail]/
>
> Changes in v2:
> - Only modify patch description information.
> - Link to v1: https://lore.kernel.org/all/[e-mail]/
> ---
>  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.

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?

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;

[...]

Zvi Effron April 16, 2025, 10:44 p.m. UTC | #2

On Wed, Apr 16, 2025 at 2:56 PM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Thu, Apr 10, 2025 at 12:03 AM Feng Yang <[e-mail]> wrote:
> >
> > From: Feng Yang <[e-mail]>
> >
> > Many conditional checks in switch-case are redundant
> > with bpf_base_func_proto and should be removed.
> >
> > Signed-off-by: Feng Yang <[e-mail]>
> > Acked-by: Song Liu <[e-mail]>
> > ---
> > Changes in v3:
> > - Only modify patch description information.
> > - Link to v2: https://lore.kernel.org/all/[e-mail]/
> >
> > Changes in v2:
> > - Only modify patch description information.
> > - Link to v1: https://lore.kernel.org/all/[e-mail]/
> > ---
> > 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;
>
> [...]
>

Feng Yang April 17, 2025, 9:52 a.m. UTC | #3

On Wed, 16 Apr 2025 14:55:43 -0700, Andrii Nakryiko <[e-mail]> wrote:

[......]
> 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?
> 
> 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)
> 

BPF_CALL_0(bpf_get_smp_processor_id)
{
	return smp_processor_id();
}
const struct bpf_func_proto bpf_get_smp_processor_id_proto = {
	.func		= bpf_get_smp_processor_id,
	.gpl_only	= false,
	.ret_type	= RET_INTEGER,
	.allow_fastcall	= true,
};
When attempting to remove bpf_get_smp_processor_id_proto,
it was observed that bpf_get_smp_processor_id is extensively used.
Should we also replace all instances of bpf_get_smp_processor_id with bpf_get_raw_cpu_id in these cases?

For example:
#define ___BPF_FUNC_MAPPER(FN, ctx...)			\
        ......
	FN(get_smp_processor_id, 8, ##ctx)		\

samples/bpf/sockex3_kern.c:
static struct globals *this_cpu_globals(void)
{
        u32 key = bpf_get_smp_processor_id();
        return bpf_map_lookup_elem(&percpu_map, &key);
}
and so on......
Thanks.
> > -       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;

Feng Yang April 17, 2025, 1:36 p.m. UTC | #4

On Thu, 17 Apr 2025 17:52:45 +0800 Feng Yang wrote:
> > >         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)
> > 

> BPF_CALL_0(bpf_get_smp_processor_id)
> {
>	return smp_processor_id();
> }
> const struct bpf_func_proto bpf_get_smp_processor_id_proto = {
>	.func		= bpf_get_smp_processor_id,
>	.gpl_only	= false,
>	.ret_type	= RET_INTEGER,
>	.allow_fastcall	= true,
> };
> When attempting to remove bpf_get_smp_processor_id_proto,
> it was observed that bpf_get_smp_processor_id is extensively used.
> Should we also replace all instances of bpf_get_smp_processor_id with bpf_get_raw_cpu_id in these cases?
>
> For example:
> #define ___BPF_FUNC_MAPPER(FN, ctx...)			\
>        ......
>	FN(get_smp_processor_id, 8, ##ctx)		\

> samples/bpf/sockex3_kern.c:
> static struct globals *this_cpu_globals(void)
> {
>        u32 key = bpf_get_smp_processor_id();
>        return bpf_map_lookup_elem(&percpu_map, &key);
> }
> and so on......
> Thanks.

I think I understand the issue now: removing this bpf_get_smp_processor_id has no impact.

For the code:
 case BPF_FUNC_get_smp_processor_id:
    return &bpf_get_raw_smp_processor_id_proto;

This configuration allows bpf_get_smp_processor_id to actually invoke the bpf_get_raw_smp_processor_id_proto function implementation.
Thanks.

Patch

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 6b07fa7081d9..c89b25344422 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1443,56 +1443,14 @@  bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 	const struct bpf_func_proto *func_proto;
 
 	switch (func_id) {
-	case BPF_FUNC_map_lookup_elem:
-		return &bpf_map_lookup_elem_proto;
-	case BPF_FUNC_map_update_elem:
-		return &bpf_map_update_elem_proto;
-	case BPF_FUNC_map_delete_elem:
-		return &bpf_map_delete_elem_proto;
-	case BPF_FUNC_map_push_elem:
-		return &bpf_map_push_elem_proto;
-	case BPF_FUNC_map_pop_elem:
-		return &bpf_map_pop_elem_proto;
-	case BPF_FUNC_map_peek_elem:
-		return &bpf_map_peek_elem_proto;
-	case BPF_FUNC_map_lookup_percpu_elem:
-		return &bpf_map_lookup_percpu_elem_proto;
-	case BPF_FUNC_ktime_get_ns:
-		return &bpf_ktime_get_ns_proto;
-	case BPF_FUNC_ktime_get_boot_ns:
-		return &bpf_ktime_get_boot_ns_proto;
-	case BPF_FUNC_tail_call:
-		return &bpf_tail_call_proto;
-	case BPF_FUNC_get_current_task:
-		return &bpf_get_current_task_proto;
-	case BPF_FUNC_get_current_task_btf:
-		return &bpf_get_current_task_btf_proto;
-	case BPF_FUNC_task_pt_regs:
-		return &bpf_task_pt_regs_proto;
 	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;
-	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;
-	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;
-	case BPF_FUNC_get_prandom_u32:
-		return &bpf_get_prandom_u32_proto;
-	case BPF_FUNC_probe_read_user:
-		return &bpf_probe_read_user_proto;
-	case BPF_FUNC_probe_read_kernel:
-		return security_locked_down(LOCKDOWN_BPF_READ_KERNEL) < 0 ?
-		       NULL : &bpf_probe_read_kernel_proto;
-	case BPF_FUNC_probe_read_user_str:
-		return &bpf_probe_read_user_str_proto;
-	case BPF_FUNC_probe_read_kernel_str:
-		return security_locked_down(LOCKDOWN_BPF_READ_KERNEL) < 0 ?
-		       NULL : &bpf_probe_read_kernel_str_proto;
 #ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
 	case BPF_FUNC_probe_read:
 		return security_locked_down(LOCKDOWN_BPF_READ_KERNEL) < 0 ?
@@ -1502,10 +1460,6 @@  bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		       NULL : &bpf_probe_read_compat_str_proto;
 #endif
 #ifdef CONFIG_CGROUPS
-	case BPF_FUNC_cgrp_storage_get:
-		return &bpf_cgrp_storage_get_proto;
-	case BPF_FUNC_cgrp_storage_delete:
-		return &bpf_cgrp_storage_delete_proto;
 	case BPF_FUNC_current_task_under_cgroup:
 		return &bpf_current_task_under_cgroup_proto;
 #endif
@@ -1513,20 +1467,6 @@  bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_send_signal_proto;
 	case BPF_FUNC_send_signal_thread:
 		return &bpf_send_signal_thread_proto;
-	case BPF_FUNC_perf_event_read_value:
-		return &bpf_perf_event_read_value_proto;
-	case BPF_FUNC_ringbuf_output:
-		return &bpf_ringbuf_output_proto;
-	case BPF_FUNC_ringbuf_reserve:
-		return &bpf_ringbuf_reserve_proto;
-	case BPF_FUNC_ringbuf_submit:
-		return &bpf_ringbuf_submit_proto;
-	case BPF_FUNC_ringbuf_discard:
-		return &bpf_ringbuf_discard_proto;
-	case BPF_FUNC_ringbuf_query:
-		return &bpf_ringbuf_query_proto;
-	case BPF_FUNC_jiffies64:
-		return &bpf_jiffies64_proto;
 	case BPF_FUNC_get_task_stack:
 		return prog->sleepable ? &bpf_get_task_stack_sleepable_proto
 				       : &bpf_get_task_stack_proto;
@@ -1534,12 +1474,6 @@  bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_copy_from_user_proto;
 	case BPF_FUNC_copy_from_user_task:
 		return &bpf_copy_from_user_task_proto;
-	case BPF_FUNC_snprintf_btf:
-		return &bpf_snprintf_btf_proto;
-	case BPF_FUNC_per_cpu_ptr:
-		return &bpf_per_cpu_ptr_proto;
-	case BPF_FUNC_this_cpu_ptr:
-		return &bpf_this_cpu_ptr_proto;
 	case BPF_FUNC_task_storage_get:
 		if (bpf_prog_check_recur(prog))
 			return &bpf_task_storage_get_recur_proto;
@@ -1548,18 +1482,12 @@  bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		if (bpf_prog_check_recur(prog))
 			return &bpf_task_storage_delete_recur_proto;
 		return &bpf_task_storage_delete_proto;
-	case BPF_FUNC_for_each_map_elem:
-		return &bpf_for_each_map_elem_proto;
-	case BPF_FUNC_snprintf:
-		return &bpf_snprintf_proto;
 	case BPF_FUNC_get_func_ip:
 		return &bpf_get_func_ip_proto_tracing;
 	case BPF_FUNC_get_branch_snapshot:
 		return &bpf_get_branch_snapshot_proto;
 	case BPF_FUNC_find_vma:
 		return &bpf_find_vma_proto;
-	case BPF_FUNC_trace_vprintk:
-		return bpf_get_trace_vprintk_proto();
 	default:
 		break;
 	}