lkml.org 
[lkml]   [2021]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16
On Fri, Oct 22, 2021 at 6:44 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Oct 20, 2021 at 8:46 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > The hard-coded 16 is used in various bpf progs. These progs get task
> > comm either via bpf_get_current_comm() or prctl() or
> > bpf_core_read_str(), all of which can work well even if the task comm size
> > is changed.
> > Below is the detailed information,
> >
> > bpf_get_current_comm:
> > progs/test_ringbuf.c
> > progs/test_ringbuf_multi.c
> >
> > prctl:
> > prog_tests/test_overhead.c
> > prog_tests/trampoline_count.c
> >
> > bpf_core_read_str:
> > progs/test_core_reloc_kernel.c
> > progs/test_sk_storage_tracing.c
> >
> > We'd better replace the hard-coded 16 with TASK_COMM_LEN_16 to make it
> > more grepable.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Petr Mladek <pmladek@suse.com>
> > ---
> > tools/testing/selftests/bpf/Makefile | 2 +-
> > tools/testing/selftests/bpf/prog_tests/ringbuf.c | 3 ++-
> > tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c | 3 ++-
> > .../testing/selftests/bpf/prog_tests/sk_storage_tracing.c | 3 ++-
> > tools/testing/selftests/bpf/prog_tests/test_overhead.c | 3 ++-
> > tools/testing/selftests/bpf/prog_tests/trampoline_count.c | 3 ++-
> > tools/testing/selftests/bpf/progs/profiler.h | 7 ++++---
> > tools/testing/selftests/bpf/progs/profiler.inc.h | 8 ++++----
> > tools/testing/selftests/bpf/progs/pyperf.h | 4 ++--
> > tools/testing/selftests/bpf/progs/strobemeta.h | 6 +++---
> > .../testing/selftests/bpf/progs/test_core_reloc_kernel.c | 3 ++-
> > tools/testing/selftests/bpf/progs/test_ringbuf.c | 3 ++-
> > tools/testing/selftests/bpf/progs/test_ringbuf_multi.c | 3 ++-
> > .../testing/selftests/bpf/progs/test_sk_storage_tracing.c | 5 +++--
> > tools/testing/selftests/bpf/progs/test_skb_helpers.c | 5 ++---
> > tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 5 +++--
> > tools/testing/selftests/bpf/progs/test_tracepoint.c | 5 +++--
> > 17 files changed, 41 insertions(+), 30 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 799b88152e9e..5e72d783d3fe 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -279,7 +279,7 @@ MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)
> >
> > CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
> > BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \
> > - -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) \
> > + -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) -I${TOOLSINCDIR} \
>
> please don't add new include paths unnecessarily. See my comment on
> another patch, if you add those new constants as enums, they will be
> automatically available in vmlinux BTF and thus in auto-generated
> vmlinux.h header (for those programs using it).

Yes, after converting it to enum, the BPF programs can get it from the
generated vmlinux.h.

> For others, I'd just
> leave hard-coded 16 or re-defined TASK_COMM_LEN_16 where appropriate.
>

It seems not all the BPF programs can include the vmlinux.h.
What we really care about here is the copy of task comm should be with
a nul terminator, if we can assure it, then the size used by the BPF
is not important.
I have checked the copy of task comm in all these BPF programs one by
one, and replaced the unsafe bpf_probe_read_kernel() with
bpf_probe_read_kernel_str(), after that change, I think we can leave
hard-coded 16 for the progs which can't include vmlinux.h.

> > -I$(abspath $(OUTPUT)/../usr/include)
> >
> > CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
> > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> > index 4706cee84360..ac82d57c09dc 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> > @@ -12,6 +12,7 @@
> > #include <sys/sysinfo.h>
> > #include <linux/perf_event.h>
> > #include <linux/ring_buffer.h>
> > +#include <linux/sched/task.h>
> > #include "test_ringbuf.lskel.h"
> >
> > #define EDONE 7777
> > @@ -22,7 +23,7 @@ struct sample {
> > int pid;
> > int seq;
> > long value;
> > - char comm[16];
> > + char comm[TASK_COMM_LEN_16];
>
> how much value is in this "grep-ability", really? I'm not convinced
> all this code churn is justified.
>
> > };
> >
> > static int sample_cnt;
> > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > index 167cd8a2edfd..f0748305ffd6 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > @@ -2,6 +2,7 @@
> > #define _GNU_SOURCE
> > #include <test_progs.h>
> > #include <sys/epoll.h>
> > +#include <linux/sched/task.h>
> > #include "test_ringbuf_multi.skel.h"
> >
> > static int duration = 0;
>
> [...]


--
Thanks
Yafang

\
 
 \ /
  Last update: 2021-10-22 08:37    [W:0.113 / U:1.908 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site