Re: [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
[off-list ref] wrote:

On Wed, Oct 20, 2021 at 8:46 PM Yafang Shao [off-list ref] wrote:

quoted

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 <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <redacted>
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.

quoted

             -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.

quoted

 };

 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