kernel/trace/bpf_trace.c | 72 ---------------------------------------- 1 file changed, 72 deletions(-)
From: Feng Yang <yangfeng@kylinos.cn>
Many conditional checks in switch-case are redundant
with bpf_base_func_proto and should be removed.
Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
Acked-by: Song Liu <song@kernel.org>
---
Changes in v3:
- Only modify patch description information.
- Link to v2: https://lore.kernel.org/all/20250408071151.229329-1-yangfeng59949@163.com/
Changes in v2:
- Only modify patch description information.
- Link to v1: https://lore.kernel.org/all/20250320032258.116156-1-yangfeng59949@163.com/
---
kernel/trace/bpf_trace.c | 72 ----------------------------------------
1 file changed, 72 deletions(-)
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;
}
--
2.43.0On Thu, Apr 10, 2025 at 12:03 AM Feng Yang <yangfeng59949@163.com> wrote: > > From: Feng Yang <yangfeng@kylinos.cn> > > Many conditional checks in switch-case are redundant > with bpf_base_func_proto and should be removed. > > Signed-off-by: Feng Yang <yangfeng@kylinos.cn> > Acked-by: Song Liu <song@kernel.org> > --- > Changes in v3: > - Only modify patch description information. > - Link to v2: https://lore.kernel.org/all/20250408071151.229329-1-yangfeng59949@163.com/ > > Changes in v2: > - Only modify patch description information. > - Link to v1: https://lore.kernel.org/all/20250320032258.116156-1-yangfeng59949@163.com/ > --- > 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; [...]
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.
On Wed, 16 Apr 2025 14:55:43 -0700, Andrii Nakryiko <andrii.nakryiko@gmail.com> 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;
On Wed, Apr 16, 2025 at 2:56 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Apr 10, 2025 at 12:03 AM Feng Yang <yangfeng59949@163.com> wrote: > > > > From: Feng Yang <yangfeng@kylinos.cn> > > > > Many conditional checks in switch-case are redundant > > with bpf_base_func_proto and should be removed. > > > > Signed-off-by: Feng Yang <yangfeng@kylinos.cn> > > Acked-by: Song Liu <song@kernel.org> > > --- > > Changes in v3: > > - Only modify patch description information. > > - Link to v2: https://lore.kernel.org/all/20250408071151.229329-1-yangfeng59949@163.com/ > > > > Changes in v2: > > - Only modify patch description information. > > - Link to v1: https://lore.kernel.org/all/20250320032258.116156-1-yangfeng59949@163.com/ > > --- > > 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; > > [...] >
On Wed, Apr 16, 2025 at 3:44 PM Zvi Effron <zeffron@riotgames.com> wrote: > > On Wed, Apr 16, 2025 at 2:56 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Thu, Apr 10, 2025 at 12:03 AM Feng Yang <yangfeng59949@163.com> wrote: > > > > > > From: Feng Yang <yangfeng@kylinos.cn> > > > > > > Many conditional checks in switch-case are redundant > > > with bpf_base_func_proto and should be removed. > > > > > > Signed-off-by: Feng Yang <yangfeng@kylinos.cn> > > > Acked-by: Song Liu <song@kernel.org> > > > --- > > > Changes in v3: > > > - Only modify patch description information. > > > - Link to v2: https://lore.kernel.org/all/20250408071151.229329-1-yangfeng59949@163.com/ > > > > > > Changes in v2: > > > - Only modify patch description information. > > > - Link to v1: https://lore.kernel.org/all/20250320032258.116156-1-yangfeng59949@163.com/ > > > --- > > > 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; > > > > [...] > >
© 2016 - 2025 Red Hat, Inc.