On Tue, Oct 26, 2021 at 5:29 AM Kees Cook [off-list ref] wrote:

On Mon, Oct 25, 2021 at 08:33:13AM +0000, Yafang Shao 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.

In these BPF programs, one thing to be improved is the
sched:sched_switch tracepoint args. As the tracepoint args are derived
from the kernel, we'd better make it same with the kernel. So the macro
TASK_COMM_LEN is converted to type enum, then all the BPF programs can
get it through BTF.

Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/sched.h                                   | 9 +++++++--
 tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 6 +++---
 tools/testing/selftests/bpf/progs/test_tracepoint.c     | 6 +++---
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index c1a927ddec64..124538db792c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h

@@ -274,8 +274,13 @@ struct task_group;

 #define get_current_state()  READ_ONCE(current->__state)

-/* Task command name length: */
-#define TASK_COMM_LEN                        16
+/*
+ * Define the task command name length as enum, then it can be visible to
+ * BPF programs.
+ */
+enum {
+     TASK_COMM_LEN = 16,
+};

 extern void scheduler_tick(void);

diff --git a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
index 00ed48672620..e9b602a6dc1b 100644
--- a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
+++ b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2018 Facebook

-#include <linux/bpf.h>
+#include <vmlinux.h>

Why is this change needed here and below?

If the BPF programs want to use the type defined in the kernel, for
example the enum we used here, we must include the vmlinux.h generated
by BTF.

quoted

 #include <bpf/bpf_helpers.h>

 #ifndef PERF_MAX_STACK_DEPTH

@@ -41,11 +41,11 @@ struct {
 /* taken from /sys/kernel/debug/tracing/events/sched/sched_switch/format */
 struct sched_switch_args {
      unsigned long long pad;
-     char prev_comm[16];
+     char prev_comm[TASK_COMM_LEN];
      int prev_pid;
      int prev_prio;
      long long prev_state;
-     char next_comm[16];
+     char next_comm[TASK_COMM_LEN];
      int next_pid;
      int next_prio;
 };

diff --git a/tools/testing/selftests/bpf/progs/test_tracepoint.c b/tools/testing/selftests/bpf/progs/test_tracepoint.c
index 4b825ee122cf..f21982681e28 100644
--- a/tools/testing/selftests/bpf/progs/test_tracepoint.c
+++ b/tools/testing/selftests/bpf/progs/test_tracepoint.c

@@ -1,17 +1,17 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2017 Facebook

-#include <linux/bpf.h>
+#include <vmlinux.h>
 #include <bpf/bpf_helpers.h>

 /* taken from /sys/kernel/debug/tracing/events/sched/sched_switch/format */
 struct sched_switch_args {
      unsigned long long pad;
-     char prev_comm[16];
+     char prev_comm[TASK_COMM_LEN];
      int prev_pid;
      int prev_prio;
      long long prev_state;
-     char next_comm[16];
+     char next_comm[TASK_COMM_LEN];
      int next_pid;
      int next_prio;
 };
--

2.17.1

--
Kees Cook

-- 
Thanks
Yafang