[PATCH v7 00/11] extend task comm from 16 to 24

[PATCH v7 00/11] extend task comm from 16 to 24

[Yafang Shao]

There're many truncated kthreads in the kernel, which may make trouble
for the user, for example, the user can't get detailed device
information from the task comm.

This patchset tries to improve this problem fundamentally by extending
the task comm size from 16 to 24, which is a very simple way. 

In order to do that, we have to do some cleanups first.

1. Make the copy of task comm always safe no matter what the task
   comm size is. For example,

      Unsafe                 Safe
      strlcpy                strscpy_pad
      strncpy                strscpy_pad
      bpf_probe_read_kernel  bpf_probe_read_kernel_str
                             bpf_core_read_str
                             bpf_get_current_comm
                             perf_event__prepare_comm
                             prctl(2)

   After this step, the comm size change won't make any trouble to the
   kernel or the in-tree tools for example perf, BPF programs.

2. Cleanup some old hard-coded 16
   Actually we don't need to convert all of them to TASK_COMM_LEN or
   TASK_COMM_LEN_16, what we really care about is if the convert can
   make the code more reasonable or easier to understand. For
   example, some in-tree tools read the comm from sched:sched_switch
   tracepoint, as it is derived from the kernel, we'd better make them
   consistent with the kernel.

3. Extend the task comm size from 16 to 24
   task_struct is growing rather regularly by 8 bytes. This size change
   should be acceptable. We used to think about extending the size for
   CONFIG_BASE_FULL only, but that would be a burden for maintenance
   and introduce code complexity.

4. Print a warning if the kthread comm is still truncated.

5. What will happen to the out-of-tree tools after this change?
   If the tool get task comm through kernel API, for example prctl(2),
   bpf_get_current_comm() and etc, then it doesn't matter how large the
   user buffer is, because it will always get a string with a nul
   terminator. While if it gets the task comm through direct string copy,
   the user tool must make sure the copied string has a nul terminator
   itself. As TASK_COMM_LEN is not exposed to userspace, there's no
   reason that it must require a fixed-size task comm.

Changes since v6:
Various suggestion from Kees:
- replace strscpy_pad() with the helper get_task_comm()
- fix typo
- replace BUILD_BUG_ON() with __must_be_array()
- don't change the size of pr_fname
- merge two samples/bpf/ patches to one patch
- keep TASK_COMM_LEN_16 per

Changes since v5:
- extend the comm size for both CONFIG_BASE_{FULL, SMALL} that could
  make the code more simple and easier to maintain.
- avoid changing too much hard-coded 16 in BPF programs per Andrii.

Changes since v4:
- introduce TASK_COMM_LEN_16 and TASK_COMM_LEN_24 per Steven
- replace hard-coded 16 with TASK_COMM_LEN_16 per Kees
- use strscpy_pad() instead of strlcpy()/strncpy() per Kees
- make perf test adopt to task comm size change per Arnaldo and Mathieu
- fix warning reported by kernel test robot

Changes since v3:
- fixes -Wstringop-truncation warning reported by kernel test robot

Changes since v2:
- avoid change UAPI code per Kees
- remove the description of out of tree code from commit log per Peter

Changes since v1:
- extend task comm to 24bytes, per Petr
- improve the warning per Petr
- make the checkpatch warning a separate patch


Yafang Shao (11):
  fs/exec: make __set_task_comm always set a nul terminated string
  fs/exec: make __get_task_comm always get a nul terminated string
  sched.h: use __must_be_array instead of BUILD_BUG_ON in get_task_comm
  drivers/infiniband: make setup_ctxt always get a nul terminated task
    comm
  fs/binfmt_elf: make prpsinfo always get a nul terminated task comm
  samples/bpf/test_overhead_kprobe_kern: make it adopt to task comm size
    change
  tools/bpf/bpftool/skeleton: make it adopt to task comm size change
  tools/perf/test: make perf test adopt to task comm size change
  tools/testing/selftests/bpf: make it adopt to task comm size change
  sched.h: extend task comm from 16 to 24
  kernel/kthread: show a warning if kthread's comm is truncated

 drivers/infiniband/hw/qib/qib.h               |  2 +-
 drivers/infiniband/hw/qib/qib_file_ops.c      |  2 +-
 fs/binfmt_elf.c                               |  2 +-
 fs/exec.c                                     |  5 ++--
 include/linux/sched.h                         | 16 +++++++-----
 kernel/kthread.c                              |  7 ++++-
 samples/bpf/offwaketime_kern.c                |  4 +--
 samples/bpf/test_overhead_kprobe_kern.c       | 11 ++++----
 samples/bpf/test_overhead_tp_kern.c           |  5 ++--
 tools/bpf/bpftool/skeleton/pid_iter.bpf.c     |  4 +--
 tools/include/linux/sched.h                   | 11 ++++++++
 tools/perf/tests/evsel-tp-sched.c             | 26 ++++++++++++++-----
 .../selftests/bpf/progs/test_stacktrace_map.c |  6 ++---
 .../selftests/bpf/progs/test_tracepoint.c     |  6 ++---
 14 files changed, 72 insertions(+), 35 deletions(-)
 create mode 100644 tools/include/linux/sched.h

-- 
2.17.1

[PATCH v7 01/11] fs/exec: make __set_task_comm always set a nul terminated string

[Yafang Shao]

Make sure the string set to task comm is always nul terminated.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 fs/exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/exec.c b/fs/exec.c
index a098c133d8d7..404156b5b314 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1224,7 +1224,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
 {
 	task_lock(tsk);
 	trace_task_rename(tsk, buf);
-	strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+	strscpy_pad(tsk->comm, buf, sizeof(tsk->comm));
 	task_unlock(tsk);
 	perf_event_comm(tsk, exec);
 }
-- 
2.17.1

[PATCH v7 02/11] fs/exec: make __get_task_comm always get a nul terminated string

[Yafang Shao]

If the dest buffer size is smaller than sizeof(tsk->comm), the buffer
will be without null ternimator, that may cause problem. We can make sure
the buffer size not smaller than comm at the callsite to avoid that
problem, but there may be callsite that we can't easily change.

Using strscpy_pad() instead of strncpy() in __get_task_comm() can make
the string always nul ternimated.

Suggested-by: Kees Cook <keescook@chromium.org>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 fs/exec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/exec.c b/fs/exec.c
index 404156b5b314..013b707d995d 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1209,7 +1209,8 @@ static int unshare_sighand(struct task_struct *me)
 char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
 {
 	task_lock(tsk);
-	strncpy(buf, tsk->comm, buf_size);
+	/* Always NUL terminated and zero-padded */
+	strscpy_pad(buf, tsk->comm, buf_size);
 	task_unlock(tsk);
 	return buf;
 }
-- 
2.17.1

[PATCH v7 03/11] sched.h: use __must_be_array instead of BUILD_BUG_ON in get_task_comm

[Yafang Shao]

Now that __get_task_comm() will truncate and pad, it's safe to use on both
too-small and too-big arrays. So it is not needed to check array length
any more. For the original goal of making sure get_task_comm() is being
used on a char array, we can use __must_be_array().

Below is the verification when I changed the dest buffer of
get_task_comm() in a driver code,

  CC [M]  drivers/infiniband/hw/qib/qib_file_ops.o
In file included from ./include/linux/bits.h:22:0,
                 from ./include/linux/ioport.h:13,
                 from ./include/linux/pci.h:31,
                 from drivers/infiniband/hw/qib/qib_file_ops.c:35:
drivers/infiniband/hw/qib/qib_file_ops.c: In function ‘setup_ctxt’:
./include/linux/build_bug.h:16:51: error: negative width in bit-field ‘<anonymous>’
 #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
                                                   ^
./include/linux/compiler.h:258:28: note: in expansion of macro ‘BUILD_BUG_ON_ZERO’
 #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
                            ^~~~~~~~~~~~~~~~~
./include/linux/sched.h:1941:23: note: in expansion of macro ‘__must_be_array’
  __get_task_comm(buf, __must_be_array(buf) + sizeof(buf), tsk)
                       ^~~~~~~~~~~~~~~
drivers/infiniband/hw/qib/qib_file_ops.c:1325:2: note: in expansion of macro ‘get_task_comm’
  get_task_comm(test, current);

It hit this warnig as expected.

Suggested-by: Kees Cook <keescook@chromium.org>
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: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/sched.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index c1a927ddec64..b9c85c52fed0 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1931,10 +1931,8 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
 }
 
 extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
-#define get_task_comm(buf, tsk) ({			\
-	BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);	\
-	__get_task_comm(buf, sizeof(buf), tsk);		\
-})
+#define get_task_comm(buf, tsk)		\
+	__get_task_comm(buf, __must_be_array(buf) + sizeof(buf), tsk)
 
 #ifdef CONFIG_SMP
 static __always_inline void scheduler_ipi(void)
-- 
2.17.1

[PATCH v7 04/11] drivers/infiniband: make setup_ctxt always get a nul terminated task comm

[Yafang Shao]

Use get_task_comm() instead of open-coded strlcpy() to make the comm always
nul terminated. As the comment above the hard-coded 16, we can replace it
with TASK_COMM_LEN, then it will adopt to the comm size change.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 drivers/infiniband/hw/qib/qib.h          | 2 +-
 drivers/infiniband/hw/qib/qib_file_ops.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib.h b/drivers/infiniband/hw/qib/qib.h
index 9363bccfc6e7..a8e1c30c370f 100644
--- a/drivers/infiniband/hw/qib/qib.h
+++ b/drivers/infiniband/hw/qib/qib.h
@@ -196,7 +196,7 @@ struct qib_ctxtdata {
 	pid_t pid;
 	pid_t subpid[QLOGIC_IB_MAX_SUBCTXT];
 	/* same size as task_struct .comm[], command that opened context */
-	char comm[16];
+	char comm[TASK_COMM_LEN];
 	/* pkeys set by this use of this ctxt */
 	u16 pkeys[4];
 	/* so file ops can get at unit */
diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
index 63854f4b6524..aa290928cf96 100644
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -1321,7 +1321,7 @@ static int setup_ctxt(struct qib_pportdata *ppd, int ctxt,
 	rcd->tid_pg_list = ptmp;
 	rcd->pid = current->pid;
 	init_waitqueue_head(&dd->rcd[ctxt]->wait);
-	strlcpy(rcd->comm, current->comm, sizeof(rcd->comm));
+	get_task_comm(rcd->comm, current);
 	ctxt_fp(fp) = rcd;
 	qib_stats.sps_ctxts++;
 	dd->freectxts--;
-- 
2.17.1

[PATCH v7 05/11] fs/binfmt_elf: make prpsinfo always get a nul terminated task comm

[Yafang Shao]

kernel test robot reported a -Wstringop-truncation warning after I
extend task comm from 16 to 24. Below is the detailed warning:

   fs/binfmt_elf.c: In function 'fill_psinfo.isra':
>> fs/binfmt_elf.c:1575:9: warning: 'strncpy' output may be truncated copying 16 bytes from a string of length 23 [-Wstringop-truncation]
    1575 |         strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Replacing the open-coded strncpy() with the helper get_task_comm() can
avoid this warning.

As the struct prpsinfo is externally parsed, for example by the crash
utility, we can't change the size of pr_fname. So I just leave it as-is.

I also verfied if it still work well when I extend the comm size to 24.
struct elf_prpsinfo is used to dump the task information in userspace
coredump or kernel vmcore. Below is the verfication of vmcore,

crash> ps
   PID    PPID  CPU       TASK        ST  %MEM     VSZ    RSS  COMM
      0      0   0  ffffffff9d21a940  RU   0.0       0      0  [swapper/0]
>     0      0   1  ffffa09e40f85e80  RU   0.0       0      0  [swapper/1]
>     0      0   2  ffffa09e40f81f80  RU   0.0       0      0  [swapper/2]
>     0      0   3  ffffa09e40f83f00  RU   0.0       0      0  [swapper/3]
>     0      0   4  ffffa09e40f80000  RU   0.0       0      0  [swapper/4]
>     0      0   5  ffffa09e40f89f80  RU   0.0       0      0  [swapper/5]
      0      0   6  ffffa09e40f8bf00  RU   0.0       0      0  [swapper/6]
>     0      0   7  ffffa09e40f88000  RU   0.0       0      0  [swapper/7]
>     0      0   8  ffffa09e40f8de80  RU   0.0       0      0  [swapper/8]
>     0      0   9  ffffa09e40f95e80  RU   0.0       0      0  [swapper/9]
>     0      0  10  ffffa09e40f91f80  RU   0.0       0      0  [swapper/10]
>     0      0  11  ffffa09e40f93f00  RU   0.0       0      0  [swapper/11]
>     0      0  12  ffffa09e40f90000  RU   0.0       0      0  [swapper/12]
>     0      0  13  ffffa09e40f9bf00  RU   0.0       0      0  [swapper/13]
>     0      0  14  ffffa09e40f98000  RU   0.0       0      0  [swapper/14]
>     0      0  15  ffffa09e40f9de80  RU   0.0       0      0  [swapper/15]

It works well as expected.

Reported-by: kernel test robot <lkp@intel.com>
Suggested-by: Kees Cook <keescook@chromium.org>
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: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 fs/binfmt_elf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index a813b70f594e..138956fd4a88 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1572,7 +1572,7 @@ static int fill_psinfo(struct elf_prpsinfo *psinfo, struct task_struct *p,
 	SET_UID(psinfo->pr_uid, from_kuid_munged(cred->user_ns, cred->uid));
 	SET_GID(psinfo->pr_gid, from_kgid_munged(cred->user_ns, cred->gid));
 	rcu_read_unlock();
-	strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
+	get_task_comm(psinfo->pr_fname, p);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v7 06/11] samples/bpf/test_overhead_kprobe_kern: make it adopt to task comm size change

[Yafang Shao]

bpf_probe_read_kernel_str() will add a nul terminator to the dst, then
we don't care about if the dst size is big enough. This patch also
replaces the hard-coded 16 with TASK_COMM_LEN to make it adopt to task
comm size change.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 samples/bpf/offwaketime_kern.c          |  4 ++--
 samples/bpf/test_overhead_kprobe_kern.c | 11 ++++++-----
 samples/bpf/test_overhead_tp_kern.c     |  5 +++--
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/samples/bpf/offwaketime_kern.c b/samples/bpf/offwaketime_kern.c
index 4866afd054da..eb4d94742e6b 100644
--- a/samples/bpf/offwaketime_kern.c
+++ b/samples/bpf/offwaketime_kern.c
@@ -113,11 +113,11 @@ static inline int update_counts(void *ctx, u32 pid, u64 delta)
 /* 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/samples/bpf/test_overhead_kprobe_kern.c b/samples/bpf/test_overhead_kprobe_kern.c
index f6d593e47037..8fdd2c9c56b2 100644
--- a/samples/bpf/test_overhead_kprobe_kern.c
+++ b/samples/bpf/test_overhead_kprobe_kern.c
@@ -6,6 +6,7 @@
  */
 #include <linux/version.h>
 #include <linux/ptrace.h>
+#include <linux/sched.h>
 #include <uapi/linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
@@ -22,17 +23,17 @@ int prog(struct pt_regs *ctx)
 {
 	struct signal_struct *signal;
 	struct task_struct *tsk;
-	char oldcomm[16] = {};
-	char newcomm[16] = {};
+	char oldcomm[TASK_COMM_LEN] = {};
+	char newcomm[TASK_COMM_LEN] = {};
 	u16 oom_score_adj;
 	u32 pid;
 
 	tsk = (void *)PT_REGS_PARM1(ctx);
 
 	pid = _(tsk->pid);
-	bpf_probe_read_kernel(oldcomm, sizeof(oldcomm), &tsk->comm);
-	bpf_probe_read_kernel(newcomm, sizeof(newcomm),
-			      (void *)PT_REGS_PARM2(ctx));
+	bpf_probe_read_kernel_str(oldcomm, sizeof(oldcomm), &tsk->comm);
+	bpf_probe_read_kernel_str(newcomm, sizeof(newcomm),
+				  (void *)PT_REGS_PARM2(ctx));
 	signal = _(tsk->signal);
 	oom_score_adj = _(signal->oom_score_adj);
 	return 0;
diff --git a/samples/bpf/test_overhead_tp_kern.c b/samples/bpf/test_overhead_tp_kern.c
index eaa32693f8fc..80edadacb692 100644
--- a/samples/bpf/test_overhead_tp_kern.c
+++ b/samples/bpf/test_overhead_tp_kern.c
@@ -4,6 +4,7 @@
  * modify it under the terms of version 2 of the GNU General Public
  * License as published by the Free Software Foundation.
  */
+#include <linux/sched.h>
 #include <uapi/linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 
@@ -11,8 +12,8 @@
 struct task_rename {
 	__u64 pad;
 	__u32 pid;
-	char oldcomm[16];
-	char newcomm[16];
+	char oldcomm[TASK_COMM_LEN];
+	char newcomm[TASK_COMM_LEN];
 	__u16 oom_score_adj;
 };
 SEC("tracepoint/task/task_rename")
-- 
2.17.1

[PATCH v7 07/11] tools/bpf/bpftool/skeleton: make it adopt to task comm size change

[Yafang Shao]

bpf_probe_read_kernel_str() will add a nul terminator to the dst, then
we don't care about if the dst size is big enough.

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: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
index d9b420972934..f70702fcb224 100644
--- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
+++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
@@ -71,8 +71,8 @@ int iter(struct bpf_iter__task_file *ctx)
 
 	e.pid = task->tgid;
 	e.id = get_obj_id(file->private_data, obj_type);
-	bpf_probe_read_kernel(&e.comm, sizeof(e.comm),
-			      task->group_leader->comm);
+	bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm),
+				  task->group_leader->comm);
 	bpf_seq_write(ctx->meta->seq, &e, sizeof(e));
 
 	return 0;
-- 
2.17.1

[PATCH v7 08/11] tools/perf/test: make perf test adopt to task comm size change

[Yafang Shao]

kernel test robot reported a perf-test failure after I extended task comm
size from 16 to 24. The failure as follows,

2021-10-13 18:00:46 sudo /usr/src/perf_selftests-x86_64-rhel-8.3-317419b91ef4eff4e2f046088201e4dc4065caa0/tools/perf/perf test 15
15: Parse sched tracepoints fields                                  : FAILED!

The reason is perf-test requires a fixed-size task comm. If we extend
task comm size to 24, it will not equil with the required size 16 in perf
test.

After some analyzation, I found perf itself can adopt to the size
change, for example, below is the output of perf-sched after I extend
comm size to 24 -

task    614 (            kthreadd:        84), nr_events: 1
task    615 (             systemd:       843), nr_events: 1
task    616 (     networkd-dispat:      1026), nr_events: 1
task    617 (             systemd:       846), nr_events: 1

$ cat /proc/843/comm
networkd-dispatcher

The task comm can be displayed correctly as expected.

Replace old hard-coded 16 with the new one can fix the warning, but we'd
better make the test accept both old and new sizes, then it can be
backward compatibility.

After this patch, the perf-test succeeds no matter task comm is 16 or
24 -

15: Parse sched tracepoints fields                                  : Ok

This patch is a preparation for the followup patch.

Reported-by: kernel test robot <oliver.sang@intel.com>
Suggested-by: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
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: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 tools/include/linux/sched.h       | 11 +++++++++++
 tools/perf/tests/evsel-tp-sched.c | 26 ++++++++++++++++++++------
 2 files changed, 31 insertions(+), 6 deletions(-)
 create mode 100644 tools/include/linux/sched.h

diff --git a/tools/include/linux/sched.h b/tools/include/linux/sched.h
new file mode 100644
index 000000000000..0d575afd7f43
--- /dev/null
+++ b/tools/include/linux/sched.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _TOOLS_LINUX_SCHED_H
+#define _TOOLS_LINUX_SCHED_H
+
+/* Keep both length for backward compatibility */
+enum {
+	TASK_COMM_LEN_16 = 16,
+	TASK_COMM_LEN = 24,
+};
+
+#endif  /* _TOOLS_LINUX_SCHED_H */
diff --git a/tools/perf/tests/evsel-tp-sched.c b/tools/perf/tests/evsel-tp-sched.c
index f9e34bd26cf3..029f2a8c8e51 100644
--- a/tools/perf/tests/evsel-tp-sched.c
+++ b/tools/perf/tests/evsel-tp-sched.c
@@ -1,11 +1,13 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/err.h>
+#include <linux/sched.h>
 #include <traceevent/event-parse.h>
 #include "evsel.h"
 #include "tests.h"
 #include "debug.h"
 
-static int evsel__test_field(struct evsel *evsel, const char *name, int size, bool should_be_signed)
+static int evsel__test_field_alt(struct evsel *evsel, const char *name,
+				 int size, int alternate_size, bool should_be_signed)
 {
 	struct tep_format_field *field = evsel__field(evsel, name);
 	int is_signed;
@@ -23,15 +25,24 @@ static int evsel__test_field(struct evsel *evsel, const char *name, int size, bo
 		ret = -1;
 	}
 
-	if (field->size != size) {
-		pr_debug("%s: \"%s\" size (%d) should be %d!\n",
+	if (field->size != size && field->size != alternate_size) {
+		pr_debug("%s: \"%s\" size (%d) should be %d",
 			 evsel->name, name, field->size, size);
+		if (alternate_size > 0)
+			pr_debug(" or %d", alternate_size);
+		pr_debug("!\n");
 		ret = -1;
 	}
 
 	return ret;
 }
 
+static int evsel__test_field(struct evsel *evsel, const char *name,
+			     int size, bool should_be_signed)
+{
+	return evsel__test_field_alt(evsel, name, size, -1, should_be_signed);
+}
+
 int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtest __maybe_unused)
 {
 	struct evsel *evsel = evsel__newtp("sched", "sched_switch");
@@ -42,7 +53,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
 		return -1;
 	}
 
-	if (evsel__test_field(evsel, "prev_comm", 16, false))
+	if (evsel__test_field_alt(evsel, "prev_comm", TASK_COMM_LEN_16,
+				  TASK_COMM_LEN, false))
 		ret = -1;
 
 	if (evsel__test_field(evsel, "prev_pid", 4, true))
@@ -54,7 +66,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
 	if (evsel__test_field(evsel, "prev_state", sizeof(long), true))
 		ret = -1;
 
-	if (evsel__test_field(evsel, "next_comm", 16, false))
+	if (evsel__test_field_alt(evsel, "next_comm", TASK_COMM_LEN_16,
+				  TASK_COMM_LEN, false))
 		ret = -1;
 
 	if (evsel__test_field(evsel, "next_pid", 4, true))
@@ -72,7 +85,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
 		return -1;
 	}
 
-	if (evsel__test_field(evsel, "comm", 16, false))
+	if (evsel__test_field_alt(evsel, "comm", TASK_COMM_LEN_16,
+				  TASK_COMM_LEN, false))
 		ret = -1;
 
 	if (evsel__test_field(evsel, "pid", 4, true))
-- 
2.17.1

[PATCH v7 09/11] tools/testing/selftests/bpf: make it adopt to task comm size change

[Yafang Shao]

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.

The BPF program which wants to use TASK_COMM_LEN should include the header
vmlinux.h. Regarding the test_stacktrace_map and test_tracepoint, as the
type defined in linux/bpf.h are also defined in vmlinux.h, so we don't
need to include linux/bpf.h again.

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: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
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 b9c85c52fed0..09ac13e54549 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>
 #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

[PATCH v7 10/11] sched.h: extend task comm from 16 to 24

[Yafang Shao]

When I was implementing a new per-cpu kthread cfs_migration, I found the
comm of it "cfs_migration/%u" is truncated due to the limitation of
TASK_COMM_LEN. For example, the comm of the percpu thread on CPU10~19 are
all with the same name "cfs_migration/1", which will confuse the user. This
issue is not critical, because we can get the corresponding CPU from the
task's Cpus_allowed. But for kthreads correspoinding to other hardware
devices, it is not easy to get the detailed device info from task comm,
for example,

    jbd2/nvme0n1p2-
    xfs-reclaim/sdf

We can also shorten the name to work around this problem, but I find
there are so many truncated kthreads:

    rcu_tasks_kthre
    rcu_tasks_rude_
    rcu_tasks_trace
    poll_mpt3sas0_s
    ext4-rsv-conver
    xfs-reclaim/sd{a, b, c, ...}
    xfs-blockgc/sd{a, b, c, ...}
    xfs-inodegc/sd{a, b, c, ...}
    audit_send_repl
    ecryptfs-kthrea
    vfio-irqfd-clea
    jbd2/nvme0n1p2-
    ...

We should improve this problem fundamentally by extending comm size to
24 bytes. task_struct is growing rather regularly by 8 bytes.

After this change, the truncated kthreads listed above will be
displayed as:

    rcu_tasks_kthread
    rcu_tasks_rude_kthread
    rcu_tasks_trace_kthread
    poll_mpt3sas0_statu
    ext4-rsv-conversion
    xfs-reclaim/sdf1
    xfs-blockgc/sdf1
    xfs-inodegc/sdf1
    audit_send_reply
    ecryptfs-kthread
    vfio-irqfd-cleanup
    jbd2/nvme0n1p2-8

As we have converted all the unsafe copy of task comm to the safe one,
this change won't make any trouble to the kernel or the in-tree tools.
The safe one and unsafe one of comm copy as follows,

  Unsafe                 Safe
  strlcpy                strscpy_pad
  strncpy                strscpy_pad
  bpf_probe_read_kernel  bpf_probe_read_kernel_str
                         bpf_core_read_str
                         bpf_get_current_comm
                         perf_event__prepare_comm
                         prctl(2)

Regarding the possible risk it may take to the out-of-tree user tools, if
the user tools get the task comm through kernel API like prctl(2),
bpf_get_current_comm() and etc, the tools still work well after this
change. While If the user tools get the task comm through direct string
copy, it must make sure the copied string should be with a nul terminator.

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: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/sched.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 09ac13e54549..a8822e26653e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -276,10 +276,11 @@ struct task_group;
 
 /*
  * Define the task command name length as enum, then it can be visible to
- * BPF programs.
+ * BPF programs. The TASK_COMM_LEN_16 is kept for backward-compitability.
  */
 enum {
-	TASK_COMM_LEN = 16,
+	TASK_COMM_LEN_16 = 16,
+	TASK_COMM_LEN = 24,
 };
 
 extern void scheduler_tick(void);
-- 
2.17.1

[PATCH v7 11/11] kernel/kthread: show a warning if kthread's comm is truncated

[Yafang Shao]

Show a warning if task comm is truncated. Below is the result
of my test case:

truncated kthread comm:I-am-a-kthread-with-a-l (pid:178) by 8 characters

As we have extended task comm to 24, all the existing in-tree
kthreads/workqueues are not truncated anymore. So this warning will only
be printed for the newly introduced one if it has a long name.

Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 kernel/kthread.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 5b37a8567168..63f38d3a4f62 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -399,12 +399,17 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 	if (!IS_ERR(task)) {
 		static const struct sched_param param = { .sched_priority = 0 };
 		char name[TASK_COMM_LEN];
+		int len;
 
 		/*
 		 * task is already visible to other tasks, so updating
 		 * COMM must be protected.
 		 */
-		vsnprintf(name, sizeof(name), namefmt, args);
+		len = vsnprintf(name, sizeof(name), namefmt, args);
+		if (len >= TASK_COMM_LEN) {
+			pr_warn("truncated kthread comm:%s (pid:%d) by %d characters\n",
+				name, task->pid, len - TASK_COMM_LEN + 1);
+		}
 		set_task_comm(task, name);
 		/*
 		 * root may have changed our (kthreadd's) priority or CPU mask.
-- 
2.17.1

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Matthew Wilcox]

On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
> There're many truncated kthreads in the kernel, which may make trouble
> for the user, for example, the user can't get detailed device
> information from the task comm.
> 
> This patchset tries to improve this problem fundamentally by extending
> the task comm size from 16 to 24, which is a very simple way. 

It can't be that simple if we're on v7 and at 11 patches!

It would be helpful if you included links to earlier postings.  I can
only find v5 and v6 in my inbox, so I fear I'm going to re-ask some
questions which were already answered.

Why can't we shorten the names of these kthreads?  You didn't
give any examples, so I can't suggest any possibilities.

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Yafang Shao]

On Mon, Nov 1, 2021 at 8:46 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
> > There're many truncated kthreads in the kernel, which may make trouble
> > for the user, for example, the user can't get detailed device
> > information from the task comm.
> >
> > This patchset tries to improve this problem fundamentally by extending
> > the task comm size from 16 to 24, which is a very simple way.
>
> It can't be that simple if we're on v7 and at 11 patches!
>

Most of the changes are because of hard-coded 16 that can't be easily grepped.
In these 11 patches, patch #1, #2, #4, #5, #6, #7 and #9 are cleanups,
which can be a different patchset.

The core changes of these patchset are patch #3, #8 and #10.

#11 can also be a seperate patch.

> It would be helpful if you included links to earlier postings.  I can
> only find v5 and v6 in my inbox, so I fear I'm going to re-ask some
> questions which were already answered.
>

v1: https://lore.kernel.org/lkml/20210929115036.4851-1-laoar.shao@gmail.com/
v2: https://lore.kernel.org/lkml/20211007120752.5195-1-laoar.shao@gmail.com/
v3: https://lore.kernel.org/lkml/20211010102429.99577-1-laoar.shao@gmail.com/
v4: https://lore.kernel.org/lkml/20211013102346.179642-1-laoar.shao@gmail.com/
v5: https://lore.kernel.org/lkml/20211021034516.4400-1-laoar.shao@gmail.com/
v6: https://lore.kernel.org/lkml/20211025083315.4752-1-laoar.shao@gmail.com/


> Why can't we shorten the names of these kthreads?  You didn't
> give any examples, so I can't suggest any possibilities.
>

Take 'jbd2/nvme0n1p2-' for example, that's a nice name, which gives a
good description via the name.
And I don't think it is a good idea to shorten its name.

-- 
Thanks
Yafang

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Petr Mladek]

On Mon 2021-11-01 06:04:08, Yafang Shao wrote:
> There're many truncated kthreads in the kernel, which may make trouble
> for the user, for example, the user can't get detailed device
> information from the task comm.
> 
> This patchset tries to improve this problem fundamentally by extending
> the task comm size from 16 to 24, which is a very simple way. 
> 
> In order to do that, we have to do some cleanups first.
> 
> 1. Make the copy of task comm always safe no matter what the task
>    comm size is. For example,
> 
>       Unsafe                 Safe
>       strlcpy                strscpy_pad
>       strncpy                strscpy_pad
>       bpf_probe_read_kernel  bpf_probe_read_kernel_str
>                              bpf_core_read_str
>                              bpf_get_current_comm
>                              perf_event__prepare_comm
>                              prctl(2)
> 
>    After this step, the comm size change won't make any trouble to the
>    kernel or the in-tree tools for example perf, BPF programs.
> 
> 2. Cleanup some old hard-coded 16
>    Actually we don't need to convert all of them to TASK_COMM_LEN or
>    TASK_COMM_LEN_16, what we really care about is if the convert can
>    make the code more reasonable or easier to understand. For
>    example, some in-tree tools read the comm from sched:sched_switch
>    tracepoint, as it is derived from the kernel, we'd better make them
>    consistent with the kernel.

The above changes make sense even if we do not extend comm[] array in
task_struct.


> 3. Extend the task comm size from 16 to 24
>    task_struct is growing rather regularly by 8 bytes. This size change
>    should be acceptable. We used to think about extending the size for
>    CONFIG_BASE_FULL only, but that would be a burden for maintenance
>    and introduce code complexity.
> 
> 4. Print a warning if the kthread comm is still truncated.
> 
> 5. What will happen to the out-of-tree tools after this change?
>    If the tool get task comm through kernel API, for example prctl(2),
>    bpf_get_current_comm() and etc, then it doesn't matter how large the
>    user buffer is, because it will always get a string with a nul
>    terminator. While if it gets the task comm through direct string copy,
>    the user tool must make sure the copied string has a nul terminator
>    itself. As TASK_COMM_LEN is not exposed to userspace, there's no
>    reason that it must require a fixed-size task comm.

The amount of code that has to be updated is really high. I am pretty
sure that there are more potential buffer overflows left.

You did not commented on the concerns in the thread
https://lore.kernel.org/all/CAADnVQKm0Ljj-w5PbkAu1ugLFnZRRPt-Vk-J7AhXxDD5xVompA@mail.gmail.com/

Several people suggested to use a more conservative approach. I mean
to keep comm[16] as is and add a new pointer to the full name. The buffer
for the long name might be dynamically allocated only when needed.

The pointer might be either in task_struct or struct kthread. It might
be used the same way as the full name stored by workqueue kthreads.

The advantage of the separate pointer:

   + would work for names longer than 32
   + will not open security holes in code

Best Regards,
Petr

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Yafang Shao]

On Mon, Nov 1, 2021 at 10:07 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Mon 2021-11-01 06:04:08, Yafang Shao wrote:
> > There're many truncated kthreads in the kernel, which may make trouble
> > for the user, for example, the user can't get detailed device
> > information from the task comm.
> >
> > This patchset tries to improve this problem fundamentally by extending
> > the task comm size from 16 to 24, which is a very simple way.
> >
> > In order to do that, we have to do some cleanups first.
> >
> > 1. Make the copy of task comm always safe no matter what the task
> >    comm size is. For example,
> >
> >       Unsafe                 Safe
> >       strlcpy                strscpy_pad
> >       strncpy                strscpy_pad
> >       bpf_probe_read_kernel  bpf_probe_read_kernel_str
> >                              bpf_core_read_str
> >                              bpf_get_current_comm
> >                              perf_event__prepare_comm
> >                              prctl(2)
> >
> >    After this step, the comm size change won't make any trouble to the
> >    kernel or the in-tree tools for example perf, BPF programs.
> >
> > 2. Cleanup some old hard-coded 16
> >    Actually we don't need to convert all of them to TASK_COMM_LEN or
> >    TASK_COMM_LEN_16, what we really care about is if the convert can
> >    make the code more reasonable or easier to understand. For
> >    example, some in-tree tools read the comm from sched:sched_switch
> >    tracepoint, as it is derived from the kernel, we'd better make them
> >    consistent with the kernel.
>
> The above changes make sense even if we do not extend comm[] array in
> task_struct.
>
>
> > 3. Extend the task comm size from 16 to 24
> >    task_struct is growing rather regularly by 8 bytes. This size change
> >    should be acceptable. We used to think about extending the size for
> >    CONFIG_BASE_FULL only, but that would be a burden for maintenance
> >    and introduce code complexity.
> >
> > 4. Print a warning if the kthread comm is still truncated.
> >
> > 5. What will happen to the out-of-tree tools after this change?
> >    If the tool get task comm through kernel API, for example prctl(2),
> >    bpf_get_current_comm() and etc, then it doesn't matter how large the
> >    user buffer is, because it will always get a string with a nul
> >    terminator. While if it gets the task comm through direct string copy,
> >    the user tool must make sure the copied string has a nul terminator
> >    itself. As TASK_COMM_LEN is not exposed to userspace, there's no
> >    reason that it must require a fixed-size task comm.
>
> The amount of code that has to be updated is really high. I am pretty
> sure that there are more potential buffer overflows left.
>
> You did not commented on the concerns in the thread
> https://lore.kernel.org/all/CAADnVQKm0Ljj-w5PbkAu1ugLFnZRRPt-Vk-J7AhXxDD5xVompA@mail.gmail.com/
>

I thought Steven[1] and  Kees[2] have already clearly explained why we
do it like that, so I didn't give any more words on it.

[1]. https://lore.kernel.org/all/20211025170503.59830a43@gandalf.local.home/
[2]. https://lore.kernel.org/all/202110251406.56F87A3522@keescook/

> Several people suggested to use a more conservative approach.

Yes, they are Al[3] and Alexei[4].

[3]. https://lore.kernel.org/lkml/YVkmaSUxbg%2FJtBHb@zeniv-ca.linux.org.uk/
[4]. https://lore.kernel.org/all/CAADnVQKm0Ljj-w5PbkAu1ugLFnZRRPt-Vk-J7AhXxDD5xVompA@mail.gmail.com/

> I mean
> to keep comm[16] as is and add a new pointer to the full name. The buffer
> for the long name might be dynamically allocated only when needed.
>

That would add a new allocation in the fork() for the threads with a long name.
I'm not sure if it is worth it.

> The pointer might be either in task_struct or struct kthread. It might
> be used the same way as the full name stored by workqueue kthreads.
>

If we decide to do it like that, I think we'd better add it in
task_struct, then it will work for all tasks.

> The advantage of the separate pointer:
>
>    + would work for names longer than 32
>    + will not open security holes in code
>

Yes, those are the advantages.  And the disadvantage of it is:

 - new allocation in fork()


-- 
Thanks
Yafang

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Petr Mladek]

On Mon 2021-11-01 22:34:30, Yafang Shao wrote:
> On Mon, Nov 1, 2021 at 10:07 PM Petr Mladek <pmladek@suse.com> wrote:
> > On Mon 2021-11-01 06:04:08, Yafang Shao wrote:
> > > 4. Print a warning if the kthread comm is still truncated.
> > >
> > > 5. What will happen to the out-of-tree tools after this change?
> > >    If the tool get task comm through kernel API, for example prctl(2),
> > >    bpf_get_current_comm() and etc, then it doesn't matter how large the
> > >    user buffer is, because it will always get a string with a nul
> > >    terminator. While if it gets the task comm through direct string copy,
> > >    the user tool must make sure the copied string has a nul terminator
> > >    itself. As TASK_COMM_LEN is not exposed to userspace, there's no
> > >    reason that it must require a fixed-size task comm.
> >
> > The amount of code that has to be updated is really high. I am pretty
> > sure that there are more potential buffer overflows left.
> >
> > You did not commented on the concerns in the thread
> > https://lore.kernel.org/all/CAADnVQKm0Ljj-w5PbkAu1ugLFnZRRPt-Vk-J7AhXxDD5xVompA@mail.gmail.com/
> >
> I thought Steven[1] and  Kees[2] have already clearly explained why we
> do it like that, so I didn't give any more words on it.
> 
> [1]. https://lore.kernel.org/all/20211025170503.59830a43@gandalf.local.home/

Steven was against switching task->comm[16] into a dynamically
allocated pointer. But he was not against storing longer names
separately.

> [2]. https://lore.kernel.org/all/202110251406.56F87A3522@keescook/

Honestly, I am a bit confused by Kees' answer. IMHO, he agreed that
switching task->comm[16] into a pointer was not worth it.

But I am not sure what he meant by "Agreed -- this is a small change
for what is already an "uncommon" corner case."


> > Several people suggested to use a more conservative approach.
> 
> Yes, they are Al[3] and Alexei[4].
> 
> [3]. https://lore.kernel.org/lkml/YVkmaSUxbg%2FJtBHb@zeniv-ca.linux.org.uk/

IMHO, Al suggested to store the long name separately and return it
by proc_task_name() when available.


> [4]. https://lore.kernel.org/all/CAADnVQKm0Ljj-w5PbkAu1ugLFnZRRPt-Vk-J7AhXxDD5xVompA@mail.gmail.com/

Alexei used dentry->d_iname as an exaxmple. struct dentry uses
d_iname[DNAME_INLINE_LEN] for short names. And dynamically
allocated d_name for long names, see *__d_alloc() implementation.

> > I mean
> > to keep comm[16] as is and add a new pointer to the full name. The buffer
> > for the long name might be dynamically allocated only when needed.
> >
> 
> That would add a new allocation in the fork() for the threads with a long name.
> I'm not sure if it is worth it.

The allocation will be done only when needed. IMHO, the performance is
important only for userspace processes. I am not aware of any kernel
subsystem that would heavily create and destroy kthreads.


> > The pointer might be either in task_struct or struct kthread. It might
> > be used the same way as the full name stored by workqueue kthreads.
> >
> 
> If we decide to do it like that, I think we'd better add it in
> task_struct, then it will work for all tasks.

Is it really needed for userspace processes? For example, ps shows
the information from /proc/*/cmdline instead.


> > The advantage of the separate pointer:
> >
> >    + would work for names longer than 32
> >    + will not open security holes in code
> >
> 
> Yes, those are the advantages.  And the disadvantage of it is:
> 
>  - new allocation in fork()

It should not be a problem if we do it only when necessary and only
for kthreads.

Best Regards,
Petr

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Steven Rostedt]

On Mon, 1 Nov 2021 17:02:12 +0100
Petr Mladek <pmladek@suse.com> wrote:

> > I thought Steven[1] and  Kees[2] have already clearly explained why we
> > do it like that, so I didn't give any more words on it.
> > 
> > [1]. https://lore.kernel.org/all/20211025170503.59830a43@gandalf.local.home/  
> 
> Steven was against switching task->comm[16] into a dynamically
> allocated pointer. But he was not against storing longer names
> separately.

Just to be clear. I was recommending that the comm[16] would still behave
like it does today. Where it is truncated. But if the name is longer, it
could be stored in a separate location if the caller wanted to know the
full name.

-- Steve

Re: [PATCH v7 09/11] tools/testing/selftests/bpf: make it adopt to task comm size change

[Andrii Nakryiko]

On Sun, Oct 31, 2021 at 11:04 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.
>
> 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.
>
> The BPF program which wants to use TASK_COMM_LEN should include the header
> vmlinux.h. Regarding the test_stacktrace_map and test_tracepoint, as the
> type defined in linux/bpf.h are also defined in vmlinux.h, so we don't
> need to include linux/bpf.h again.
>
> 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: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Petr Mladek <pmladek@suse.com>
> ---

LGTM, thanks.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  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(-)
>

[...]

Re: [PATCH v7 07/11] tools/bpf/bpftool/skeleton: make it adopt to task comm size change

[Andrii Nakryiko]

On Sun, Oct 31, 2021 at 11:04 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> bpf_probe_read_kernel_str() will add a nul terminator to the dst, then
> we don't care about if the dst size is big enough.
>
> 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: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Petr Mladek <pmladek@suse.com>
> ---

LGTM.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> index d9b420972934..f70702fcb224 100644
> --- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
> @@ -71,8 +71,8 @@ int iter(struct bpf_iter__task_file *ctx)
>
>         e.pid = task->tgid;
>         e.id = get_obj_id(file->private_data, obj_type);
> -       bpf_probe_read_kernel(&e.comm, sizeof(e.comm),
> -                             task->group_leader->comm);
> +       bpf_probe_read_kernel_str(&e.comm, sizeof(e.comm),
> +                                 task->group_leader->comm);
>         bpf_seq_write(ctx->meta->seq, &e, sizeof(e));
>
>         return 0;
> --
> 2.17.1
>

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Yafang Shao]

On Tue, Nov 2, 2021 at 12:02 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Mon 2021-11-01 22:34:30, Yafang Shao wrote:
> > On Mon, Nov 1, 2021 at 10:07 PM Petr Mladek <pmladek@suse.com> wrote:
> > > On Mon 2021-11-01 06:04:08, Yafang Shao wrote:
> > > > 4. Print a warning if the kthread comm is still truncated.
> > > >
> > > > 5. What will happen to the out-of-tree tools after this change?
> > > >    If the tool get task comm through kernel API, for example prctl(2),
> > > >    bpf_get_current_comm() and etc, then it doesn't matter how large the
> > > >    user buffer is, because it will always get a string with a nul
> > > >    terminator. While if it gets the task comm through direct string copy,
> > > >    the user tool must make sure the copied string has a nul terminator
> > > >    itself. As TASK_COMM_LEN is not exposed to userspace, there's no
> > > >    reason that it must require a fixed-size task comm.
> > >
> > > The amount of code that has to be updated is really high. I am pretty
> > > sure that there are more potential buffer overflows left.
> > >
> > > You did not commented on the concerns in the thread
> > > https://lore.kernel.org/all/CAADnVQKm0Ljj-w5PbkAu1ugLFnZRRPt-Vk-J7AhXxDD5xVompA@mail.gmail.com/
> > >
> > I thought Steven[1] and  Kees[2] have already clearly explained why we
> > do it like that, so I didn't give any more words on it.
> >
> > [1]. https://lore.kernel.org/all/20211025170503.59830a43@gandalf.local.home/
>
> Steven was against switching task->comm[16] into a dynamically
> allocated pointer. But he was not against storing longer names
> separately.
>
> > [2]. https://lore.kernel.org/all/202110251406.56F87A3522@keescook/
>
> Honestly, I am a bit confused by Kees' answer. IMHO, he agreed that
> switching task->comm[16] into a pointer was not worth it.
>
> But I am not sure what he meant by "Agreed -- this is a small change
> for what is already an "uncommon" corner case."
>
>
> > > Several people suggested to use a more conservative approach.
> >
> > Yes, they are Al[3] and Alexei[4].
> >
> > [3]. https://lore.kernel.org/lkml/YVkmaSUxbg%2FJtBHb@zeniv-ca.linux.org.uk/
>
> IMHO, Al suggested to store the long name separately and return it
> by proc_task_name() when available.
>
>
> > [4]. https://lore.kernel.org/all/CAADnVQKm0Ljj-w5PbkAu1ugLFnZRRPt-Vk-J7AhXxDD5xVompA@mail.gmail.com/
>
> Alexei used dentry->d_iname as an exaxmple. struct dentry uses
> d_iname[DNAME_INLINE_LEN] for short names. And dynamically
> allocated d_name for long names, see *__d_alloc() implementation.
>

Thanks for the summary.
So with Stenven's new reply[1], the opinion in common is storing long
names into a separate place. And no one is against it now.

[1]. https://lore.kernel.org/lkml/20211101120636.3cfc5afa@gandalf.local.home/

> > > I mean
> > > to keep comm[16] as is and add a new pointer to the full name. The buffer
> > > for the long name might be dynamically allocated only when needed.
> > >
> >
> > That would add a new allocation in the fork() for the threads with a long name.
> > I'm not sure if it is worth it.
>
> The allocation will be done only when needed. IMHO, the performance is
> important only for userspace processes. I am not aware of any kernel
> subsystem that would heavily create and destroy kthreads.
>

XFS may create many kthreads with longer names, especially if there're
many partitions in the disk.
For example,
    xfs-reclaim/sd{a, b, c, ...}
    xfs-blockgc/sd{a, b, c, ...}
    xfs-inodegc/sd{a, b, c, ...}

They are supposed to be created at boot time, and shouldn't be heavily
created and destroyed.

>
> > > The pointer might be either in task_struct or struct kthread. It might
> > > be used the same way as the full name stored by workqueue kthreads.
> > >
> >
> > If we decide to do it like that, I think we'd better add it in
> > task_struct, then it will work for all tasks.
>
> Is it really needed for userspace processes? For example, ps shows
> the information from /proc/*/cmdline instead.
>

Right. The userspace processes can be obtained from /proc/*/cmdline.

>
> > > The advantage of the separate pointer:
> > >
> > >    + would work for names longer than 32
> > >    + will not open security holes in code
> > >
> >
> > Yes, those are the advantages.  And the disadvantage of it is:
> >
> >  - new allocation in fork()
>
> It should not be a problem if we do it only when necessary and only
> for kthreads.
>

So if no one against, I will do it in two steps,

1. Send the task comm cleanups in a separate patchset named "task comm cleanups"
    This patchset includes patch #1, #2, #4,  #5, #6, #7 and #9.
    Cleaning them up can make it less error prone, and it will be
helpful if we want to extend task comm in the future :)

2.  Keep the current comm[16] as-is and introduce a separate pointer
to store kthread's long name
     Now we only care about kthread, so we can put the pointer into a
kthread specific struct.
     For example in the struct kthread, or in kthread->data (which may
conflict with workqueue).

     And then dynamically allocate a longer name if it is truncated,
for example,
     __kthread_create_on_node
         len = vsnprintf(name, sizeof(name), namefmt, args);
         if (len >= TASK_COMM_LEN) {
             /* create a longer name */
         }

     And then we modify proc_task_name(), so the user can get
kthread's longer name via /proc/[pid]/comm.

     And then free the allocated memory when the kthread is destroyed.

--
Thanks
Yafang

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Steven Rostedt]

On Tue, 2 Nov 2021 09:09:50 +0800
Yafang Shao <laoar.shao@gmail.com> wrote:

> So if no one against, I will do it in two steps,
> 
> 1. Send the task comm cleanups in a separate patchset named "task comm cleanups"
>     This patchset includes patch #1, #2, #4,  #5, #6, #7 and #9.
>     Cleaning them up can make it less error prone, and it will be
> helpful if we want to extend task comm in the future :)

Agreed.

> 
> 2.  Keep the current comm[16] as-is and introduce a separate pointer
> to store kthread's long name

I'm OK with this. Hopefully more would chime in too.

>      Now we only care about kthread, so we can put the pointer into a
> kthread specific struct.
>      For example in the struct kthread, or in kthread->data (which may
> conflict with workqueue).

No, add a new field to the structure. "full_name" or something like that.
I'm guessing it should be NULL if the name fits in TASK_COMM_LEN and
allocated if the name had to be truncated.

Do not overload data with this. That will just make things confusing.
There's not that many kthreads, where an addition of an 8 byte pointer is
going to cause issues.

> 
>      And then dynamically allocate a longer name if it is truncated,
> for example,
>      __kthread_create_on_node
>          len = vsnprintf(name, sizeof(name), namefmt, args);
>          if (len >= TASK_COMM_LEN) {
>              /* create a longer name */

And make sure you have it fail the kthread allocation if it fails to
allocate.

>          }
> 
>      And then we modify proc_task_name(), so the user can get
> kthread's longer name via /proc/[pid]/comm.

Agreed.

> 
>      And then free the allocated memory when the kthread is destroyed.

Correct.

Thanks,

-- Steve

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Yafang Shao]

On Tue, Nov 2, 2021 at 9:18 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 2 Nov 2021 09:09:50 +0800
> Yafang Shao <laoar.shao@gmail.com> wrote:
>
> > So if no one against, I will do it in two steps,
> >
> > 1. Send the task comm cleanups in a separate patchset named "task comm cleanups"
> >     This patchset includes patch #1, #2, #4,  #5, #6, #7 and #9.
> >     Cleaning them up can make it less error prone, and it will be
> > helpful if we want to extend task comm in the future :)
>
> Agreed.
>
> >
> > 2.  Keep the current comm[16] as-is and introduce a separate pointer
> > to store kthread's long name
>
> I'm OK with this. Hopefully more would chime in too.
>
> >      Now we only care about kthread, so we can put the pointer into a
> > kthread specific struct.
> >      For example in the struct kthread, or in kthread->data (which may
> > conflict with workqueue).
>
> No, add a new field to the structure. "full_name" or something like that.
> I'm guessing it should be NULL if the name fits in TASK_COMM_LEN and
> allocated if the name had to be truncated.
>
> Do not overload data with this. That will just make things confusing.
> There's not that many kthreads, where an addition of an 8 byte pointer is
> going to cause issues.
>

Sure, I will add a new field named "full_name", which only be
allocated if the kthread's comm is truncated.

> >
> >      And then dynamically allocate a longer name if it is truncated,
> > for example,
> >      __kthread_create_on_node
> >          len = vsnprintf(name, sizeof(name), namefmt, args);
> >          if (len >= TASK_COMM_LEN) {
> >              /* create a longer name */
>
> And make sure you have it fail the kthread allocation if it fails to
> allocate.
>
> >          }
> >
> >      And then we modify proc_task_name(), so the user can get
> > kthread's longer name via /proc/[pid]/comm.
>
> Agreed.
>
> >
> >      And then free the allocated memory when the kthread is destroyed.
>
> Correct.
>
> Thanks,
>
> -- Steve



-- 
Thanks
Yafang

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Petr Mladek]

On Tue 2021-11-02 09:26:35, Yafang Shao wrote:
> On Tue, Nov 2, 2021 at 9:18 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Tue, 2 Nov 2021 09:09:50 +0800
> > Yafang Shao <laoar.shao@gmail.com> wrote:
> > >      Now we only care about kthread, so we can put the pointer into a
> > > kthread specific struct.
> > >      For example in the struct kthread, or in kthread->data (which may
> > > conflict with workqueue).
> >
> > No, add a new field to the structure. "full_name" or something like that.
> > I'm guessing it should be NULL if the name fits in TASK_COMM_LEN and
> > allocated if the name had to be truncated.
> >
> > Do not overload data with this. That will just make things confusing.
> > There's not that many kthreads, where an addition of an 8 byte pointer is
> > going to cause issues.
> 
> Sure, I will add a new field named "full_name", which only be
> allocated if the kthread's comm is truncated.

The plan looks good to me.

One more thing. It should obsolete the workqueue-specific solution.
It would be great to clean up the workqueue code as the next step.

Best Regards,
Petr

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[David Hildenbrand]

>> 2.  Keep the current comm[16] as-is and introduce a separate pointer
>> to store kthread's long name
> 
> I'm OK with this. Hopefully more would chime in too.

What you propose sounds sane to me.


-- 
Thanks,

David / dhildenb

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Yafang Shao]

On Tue, Nov 2, 2021 at 3:56 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Tue 2021-11-02 09:26:35, Yafang Shao wrote:
> > On Tue, Nov 2, 2021 at 9:18 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> > > On Tue, 2 Nov 2021 09:09:50 +0800
> > > Yafang Shao <laoar.shao@gmail.com> wrote:
> > > >      Now we only care about kthread, so we can put the pointer into a
> > > > kthread specific struct.
> > > >      For example in the struct kthread, or in kthread->data (which may
> > > > conflict with workqueue).
> > >
> > > No, add a new field to the structure. "full_name" or something like that.
> > > I'm guessing it should be NULL if the name fits in TASK_COMM_LEN and
> > > allocated if the name had to be truncated.
> > >
> > > Do not overload data with this. That will just make things confusing.
> > > There's not that many kthreads, where an addition of an 8 byte pointer is
> > > going to cause issues.
> >
> > Sure, I will add a new field named "full_name", which only be
> > allocated if the kthread's comm is truncated.
>
> The plan looks good to me.
>
> One more thing. It should obsolete the workqueue-specific solution.
> It would be great to clean up the workqueue code as the next step.
>

Agreed. The worker comm can be replaced by the new kthread full_name.
I will do it in the next step.

-- 
Thanks
Yafang

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Michał Mirosław]

On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
> There're many truncated kthreads in the kernel, which may make trouble
> for the user, for example, the user can't get detailed device
> information from the task comm.
> 
> This patchset tries to improve this problem fundamentally by extending
> the task comm size from 16 to 24, which is a very simple way. 
[...]

Hi,

I've tried something like this a few years back. My attempt got mostly
lost in the mailing lists, but I'm still carrying the patches in my
tree [1]. My target was userspace thread names, and it turned out more
involved than I had time for.

[1] https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a
    and its parents

Best Regards
Michał Mirosław

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Yafang Shao]

On Thu, Nov 4, 2021 at 9:37 AM Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
>
> On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
> > There're many truncated kthreads in the kernel, which may make trouble
> > for the user, for example, the user can't get detailed device
> > information from the task comm.
> >
> > This patchset tries to improve this problem fundamentally by extending
> > the task comm size from 16 to 24, which is a very simple way.
> [...]
>
> Hi,
>
> I've tried something like this a few years back. My attempt got mostly
> lost in the mailing lists, but I'm still carrying the patches in my
> tree [1]. My target was userspace thread names, and it turned out more
> involved than I had time for.
>
> [1] https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a
>     and its parents
>

Hi Michal,

Thanks for the information.

I have looked through your patches.  It seems to contain six patches
now and can be divided into three parts per my understanding.

1. extend task comm len
This parts contains below 4 patches:
[prctl: prepare for bigger
TASK_COMM_LEN](https://rere.qmqm.pl/git/?p=linux;a=commit;h=cfd99db9cf911bb4d106889aeba1dfe89b6527d0)
[bluetooth: prepare for bigger
TASK_COMM_LEN](https://rere.qmqm.pl/git/?p=linux;a=commit;h=ba2805f5196865b81cc6fc938ea53af2c7c2c892)
[taskstats: prepare for bigger
TASK_COMM_LEN](https://rere.qmqm.pl/git/?p=linux;a=commit;h=4d29bfedc57b36607915a0171f4864ec504908ca)
[mm: make TASK_COMM_LEN
configurable](https://rere.qmqm.pl/git/?p=linux;a=commit;h=362acc35582445174589184c738c4d86ec7d174b)

What kind of userspace issues makes you extend the task comm length ?
Why not just use /proc/[pid]/cmdline ?

2.  A fix
Below patch:
[procfs: signal /proc/PID/comm write
truncation](https://rere.qmqm.pl/git/?p=linux;a=commit;h=d72027388d4d95db5438a7a574e0a03ae4b5d6d7)

It seems this patch is incomplete ?   I don't know what it means to do.

3. A feature provided for pthread_getname_np
Below patch:
[procfs: lseek(/proc/PID/comm, 0,
SEEK_END)](https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a)

It seems this patch is useful. With this patch the userspace can
directly get the TASK_COMM_LEN through the API.

-- 
Thanks
Yafang

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Michał Mirosław]

On Fri, Nov 05, 2021 at 02:34:58PM +0800, Yafang Shao wrote:
> On Thu, Nov 4, 2021 at 9:37 AM Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> >
> > On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
> > > There're many truncated kthreads in the kernel, which may make trouble
> > > for the user, for example, the user can't get detailed device
> > > information from the task comm.
> > >
> > > This patchset tries to improve this problem fundamentally by extending
> > > the task comm size from 16 to 24, which is a very simple way.
> > [...]
> >
> > Hi,
> >
> > I've tried something like this a few years back. My attempt got mostly
> > lost in the mailing lists, but I'm still carrying the patches in my
> > tree [1]. My target was userspace thread names, and it turned out more
> > involved than I had time for.
> >
> > [1] https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a
> >     and its parents
> >
> 
> Hi Michal,
> 
> Thanks for the information.
> 
> I have looked through your patches.  It seems to contain six patches
> now and can be divided into three parts per my understanding.
> 
> 1. extend task comm len
> This parts contains below 4 patches:
> [prctl: prepare for bigger
> TASK_COMM_LEN](https://rere.qmqm.pl/git/?p=linux;a=commit;h=cfd99db9cf911bb4d106889aeba1dfe89b6527d0)
> [bluetooth: prepare for bigger
> TASK_COMM_LEN](https://rere.qmqm.pl/git/?p=linux;a=commit;h=ba2805f5196865b81cc6fc938ea53af2c7c2c892)
> [taskstats: prepare for bigger
> TASK_COMM_LEN](https://rere.qmqm.pl/git/?p=linux;a=commit;h=4d29bfedc57b36607915a0171f4864ec504908ca)
> [mm: make TASK_COMM_LEN
> configurable](https://rere.qmqm.pl/git/?p=linux;a=commit;h=362acc35582445174589184c738c4d86ec7d174b)
> 
> What kind of userspace issues makes you extend the task comm length ?
> Why not just use /proc/[pid]/cmdline ?

This was to enable longer thread names (as set by pthread_setname_np()).
Currently its 16 bytes, and that's too short for e.g. Chrome's or Firefox'es
threads. I believe that FreeBSD has 32-byte limit and so I expect that
major portable code is already prepared for bigger thread names.

> 2.  A fix
> Below patch:
> [procfs: signal /proc/PID/comm write
> truncation](https://rere.qmqm.pl/git/?p=linux;a=commit;h=d72027388d4d95db5438a7a574e0a03ae4b5d6d7)
> 
> It seems this patch is incomplete ?   I don't know what it means to do.

Currently writes to /proc/PID/comm are silently truncated. This patch
makes the write() call return the actual number of bytes actually written
and on subsequent calls return -ENOSPC. glibc checks the length in
pthread_setname_np() before write(), so the change is not currently
relevant for it. I don't know/remember what other runtimes do, though.

> 3. A feature provided for pthread_getname_np
> Below patch:
> [procfs: lseek(/proc/PID/comm, 0,
> SEEK_END)](https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a)
> 
> It seems this patch is useful. With this patch the userspace can
> directly get the TASK_COMM_LEN through the API.

This one I'm not really fond of because it abuses lseek() in that it
doesn't move the write pointer. But in case of /proc files this normally
would return EINVAL anyway.

Best Regards
Michał Mirosław

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Yafang Shao]

On Sat, Nov 6, 2021 at 7:57 AM Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
>
> On Fri, Nov 05, 2021 at 02:34:58PM +0800, Yafang Shao wrote:
> > On Thu, Nov 4, 2021 at 9:37 AM Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> > >
> > > On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
> > > > There're many truncated kthreads in the kernel, which may make trouble
> > > > for the user, for example, the user can't get detailed device
> > > > information from the task comm.
> > > >
> > > > This patchset tries to improve this problem fundamentally by extending
> > > > the task comm size from 16 to 24, which is a very simple way.
> > > [...]
> > >
> > > Hi,
> > >
> > > I've tried something like this a few years back. My attempt got mostly
> > > lost in the mailing lists, but I'm still carrying the patches in my
> > > tree [1]. My target was userspace thread names, and it turned out more
> > > involved than I had time for.
> > >
> > > [1] https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a
> > >     and its parents
> > >
> >
> > Hi Michal,
> >
> > Thanks for the information.
> >
> > I have looked through your patches.  It seems to contain six patches
> > now and can be divided into three parts per my understanding.
> >
> > 1. extend task comm len
> > This parts contains below 4 patches:
> > [prctl: prepare for bigger
> > TASK_COMM_LEN](https://rere.qmqm.pl/git/?p=linux;a=commit;h=cfd99db9cf911bb4d106889aeba1dfe89b6527d0)
> > [bluetooth: prepare for bigger
> > TASK_COMM_LEN](https://rere.qmqm.pl/git/?p=linux;a=commit;h=ba2805f5196865b81cc6fc938ea53af2c7c2c892)
> > [taskstats: prepare for bigger
> > TASK_COMM_LEN](https://rere.qmqm.pl/git/?p=linux;a=commit;h=4d29bfedc57b36607915a0171f4864ec504908ca)
> > [mm: make TASK_COMM_LEN
> > configurable](https://rere.qmqm.pl/git/?p=linux;a=commit;h=362acc35582445174589184c738c4d86ec7d174b)
> >
> > What kind of userspace issues makes you extend the task comm length ?
> > Why not just use /proc/[pid]/cmdline ?
>
> This was to enable longer thread names (as set by pthread_setname_np()).
> Currently its 16 bytes, and that's too short for e.g. Chrome's or Firefox'es
> threads. I believe that FreeBSD has 32-byte limit and so I expect that
> major portable code is already prepared for bigger thread names.
>

The comm len in FreeBSD is (19 + 1) bytes[1], but that is still larger
than Linux :)
The task comm is short for many applications, that is why cmdline is
introduced per my understanding, but pthread_{set, get}name_np() is
reading/writing the comm or via prctl(2) rather than reading/writing
the cmdline...

Is the truncated Chrome or Firefox thread comm really harmful or is
extending the task comm just for portable?
Could you pls show me some examples if the short comm is really harmful?

Per my understanding, if the short comm is harmful to applications
then it is worth extending it.
But if it is only for portable code, it may not be worth extending it.

[1]. https://github.com/freebsd/freebsd-src/blob/main/sys/sys/param.h#L126

> > 2.  A fix
> > Below patch:
> > [procfs: signal /proc/PID/comm write
> > truncation](https://rere.qmqm.pl/git/?p=linux;a=commit;h=d72027388d4d95db5438a7a574e0a03ae4b5d6d7)
> >
> > It seems this patch is incomplete ?   I don't know what it means to do.
>
> Currently writes to /proc/PID/comm are silently truncated. This patch
> makes the write() call return the actual number of bytes actually written
> and on subsequent calls return -ENOSPC. glibc checks the length in
> pthread_setname_np() before write(), so the change is not currently
> relevant for it. I don't know/remember what other runtimes do, though.
>
> > 3. A feature provided for pthread_getname_np
> > Below patch:
> > [procfs: lseek(/proc/PID/comm, 0,
> > SEEK_END)](https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a)
> >
> > It seems this patch is useful. With this patch the userspace can
> > directly get the TASK_COMM_LEN through the API.
>
> This one I'm not really fond of because it abuses lseek() in that it
> doesn't move the write pointer. But in case of /proc files this normally
> would return EINVAL anyway.
>

Another possible way is introducing a new PR_GET_COMM_LEN for
prctl(2), but I'm not sure if it is worth it.

-- 
Thanks
Yafang

Re: [PATCH v7 00/11] extend task comm from 16 to 24

[Michał Mirosław]

On Sat, Nov 06, 2021 at 05:12:24PM +0800, Yafang Shao wrote:
> On Sat, Nov 6, 2021 at 7:57 AM Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> >
> > On Fri, Nov 05, 2021 at 02:34:58PM +0800, Yafang Shao wrote:
> > > On Thu, Nov 4, 2021 at 9:37 AM Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> > > >
> > > > On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
> > > > > There're many truncated kthreads in the kernel, which may make trouble
> > > > > for the user, for example, the user can't get detailed device
> > > > > information from the task comm.
> > > > >
> > > > > This patchset tries to improve this problem fundamentally by extending
> > > > > the task comm size from 16 to 24, which is a very simple way.
> > > > [...]
> > > >
> > > > Hi,
> > > >
> > > > I've tried something like this a few years back. My attempt got mostly
> > > > lost in the mailing lists, but I'm still carrying the patches in my
> > > > tree [1]. My target was userspace thread names, and it turned out more
> > > > involved than I had time for.
> > > >
> > > > [1] https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a
> > > >     and its parents
> > > >
> > >
> > > Hi Michal,
> > >
> > > Thanks for the information.
> > >
> > > I have looked through your patches.  It seems to contain six patches
> > > now and can be divided into three parts per my understanding.
> > >
> > > 1. extend task comm len
> > > This parts contains below 4 patches:
> > > [prctl: prepare for bigger
> > > TASK_COMM_LEN](https://rere.qmqm.pl/git/?p=linux;a=commit;h=cfd99db9cf911bb4d106889aeba1dfe89b6527d0)
> > > [bluetooth: prepare for bigger
> > > TASK_COMM_LEN](https://rere.qmqm.pl/git/?p=linux;a=commit;h=ba2805f5196865b81cc6fc938ea53af2c7c2c892)
> > > [taskstats: prepare for bigger
> > > TASK_COMM_LEN](https://rere.qmqm.pl/git/?p=linux;a=commit;h=4d29bfedc57b36607915a0171f4864ec504908ca)
> > > [mm: make TASK_COMM_LEN
> > > configurable](https://rere.qmqm.pl/git/?p=linux;a=commit;h=362acc35582445174589184c738c4d86ec7d174b)
> > >
> > > What kind of userspace issues makes you extend the task comm length ?
> > > Why not just use /proc/[pid]/cmdline ?
> >
> > This was to enable longer thread names (as set by pthread_setname_np()).
> > Currently its 16 bytes, and that's too short for e.g. Chrome's or Firefox'es
> > threads. I believe that FreeBSD has 32-byte limit and so I expect that
> > major portable code is already prepared for bigger thread names.
> >
> 
> The comm len in FreeBSD is (19 + 1) bytes[1], but that is still larger
> than Linux :)
> The task comm is short for many applications, that is why cmdline is
> introduced per my understanding, but pthread_{set, get}name_np() is
> reading/writing the comm or via prctl(2) rather than reading/writing
> the cmdline...
> 
> Is the truncated Chrome or Firefox thread comm really harmful or is
> extending the task comm just for portable?
> Could you pls show me some examples if the short comm is really harmful?
> 
> Per my understanding, if the short comm is harmful to applications
> then it is worth extending it.
> But if it is only for portable code, it may not be worth extending it.
> 
> [1]. https://github.com/freebsd/freebsd-src/blob/main/sys/sys/param.h#L126

I don't think it is harmful as in exposing a bug or something. It's just
inconvenient when debugging a system where you can't differentiate
between threads because their names have been cut too short.

Best Regards
Michał Mirosław

Re: [PATCH v7 08/11] tools/perf/test: make perf test adopt to task comm size change

[Arnaldo Carvalho de Melo]

Em Mon, Nov 01, 2021 at 06:04:16AM +0000, Yafang Shao escreveu:
> kernel test robot reported a perf-test failure after I extended task comm
> size from 16 to 24. The failure as follows,
> 
> 2021-10-13 18:00:46 sudo /usr/src/perf_selftests-x86_64-rhel-8.3-317419b91ef4eff4e2f046088201e4dc4065caa0/tools/perf/perf test 15
> 15: Parse sched tracepoints fields                                  : FAILED!
> 
> The reason is perf-test requires a fixed-size task comm. If we extend
> task comm size to 24, it will not equil with the required size 16 in perf
> test.
> 
> After some analyzation, I found perf itself can adopt to the size
> change, for example, below is the output of perf-sched after I extend
> comm size to 24 -
> 
> task    614 (            kthreadd:        84), nr_events: 1
> task    615 (             systemd:       843), nr_events: 1
> task    616 (     networkd-dispat:      1026), nr_events: 1
> task    617 (             systemd:       846), nr_events: 1
> 
> $ cat /proc/843/comm
> networkd-dispatcher
> 
> The task comm can be displayed correctly as expected.
> 
> Replace old hard-coded 16 with the new one can fix the warning, but we'd
> better make the test accept both old and new sizes, then it can be
> backward compatibility.
> 
> After this patch, the perf-test succeeds no matter task comm is 16 or
> 24 -
> 
> 15: Parse sched tracepoints fields                                  : Ok
> 
> This patch is a preparation for the followup patch.
> 
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Suggested-by: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> 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: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Petr Mladek <pmladek@suse.com>
> ---
>  tools/include/linux/sched.h       | 11 +++++++++++
>  tools/perf/tests/evsel-tp-sched.c | 26 ++++++++++++++++++++------
>  2 files changed, 31 insertions(+), 6 deletions(-)
>  create mode 100644 tools/include/linux/sched.h
> 
> diff --git a/tools/include/linux/sched.h b/tools/include/linux/sched.h
> new file mode 100644
> index 000000000000..0d575afd7f43
> --- /dev/null
> +++ b/tools/include/linux/sched.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _TOOLS_LINUX_SCHED_H
> +#define _TOOLS_LINUX_SCHED_H
> +
> +/* Keep both length for backward compatibility */
> +enum {
> +	TASK_COMM_LEN_16 = 16,
> +	TASK_COMM_LEN = 24,
> +};
> +

I don't think this is a good idea, to have it in tools/include/linux/,
we have /usr/include/linux/sched.h, this may end up confusing the build
at some point as your proposal is for a trimmed down header while what
is in /usr/include/linux/sched.h doesn't have just this.

But since we're using enums for this, we can't check for it with:

#ifdef TASK_COMM_LEN_16
#define TASK_COMM_LEN_16 16
#endif

ditto for TASK_COMM_LEN and be future proof, so I'd say just use
hardcoded values in tools/perf/tests/evsel-tp-sched.c?

- Arnaldo

> +#endif  /* _TOOLS_LINUX_SCHED_H */
> diff --git a/tools/perf/tests/evsel-tp-sched.c b/tools/perf/tests/evsel-tp-sched.c
> index f9e34bd26cf3..029f2a8c8e51 100644
> --- a/tools/perf/tests/evsel-tp-sched.c
> +++ b/tools/perf/tests/evsel-tp-sched.c
> @@ -1,11 +1,13 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/err.h>
> +#include <linux/sched.h>
>  #include <traceevent/event-parse.h>
>  #include "evsel.h"
>  #include "tests.h"
>  #include "debug.h"
>  
> -static int evsel__test_field(struct evsel *evsel, const char *name, int size, bool should_be_signed)
> +static int evsel__test_field_alt(struct evsel *evsel, const char *name,
> +				 int size, int alternate_size, bool should_be_signed)
>  {
>  	struct tep_format_field *field = evsel__field(evsel, name);
>  	int is_signed;
> @@ -23,15 +25,24 @@ static int evsel__test_field(struct evsel *evsel, const char *name, int size, bo
>  		ret = -1;
>  	}
>  
> -	if (field->size != size) {
> -		pr_debug("%s: \"%s\" size (%d) should be %d!\n",
> +	if (field->size != size && field->size != alternate_size) {
> +		pr_debug("%s: \"%s\" size (%d) should be %d",
>  			 evsel->name, name, field->size, size);
> +		if (alternate_size > 0)
> +			pr_debug(" or %d", alternate_size);
> +		pr_debug("!\n");
>  		ret = -1;
>  	}
>  
>  	return ret;
>  }
>  
> +static int evsel__test_field(struct evsel *evsel, const char *name,
> +			     int size, bool should_be_signed)
> +{
> +	return evsel__test_field_alt(evsel, name, size, -1, should_be_signed);
> +}
> +
>  int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtest __maybe_unused)
>  {
>  	struct evsel *evsel = evsel__newtp("sched", "sched_switch");
> @@ -42,7 +53,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
>  		return -1;
>  	}
>  
> -	if (evsel__test_field(evsel, "prev_comm", 16, false))
> +	if (evsel__test_field_alt(evsel, "prev_comm", TASK_COMM_LEN_16,
> +				  TASK_COMM_LEN, false))
>  		ret = -1;
>  
>  	if (evsel__test_field(evsel, "prev_pid", 4, true))
> @@ -54,7 +66,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
>  	if (evsel__test_field(evsel, "prev_state", sizeof(long), true))
>  		ret = -1;
>  
> -	if (evsel__test_field(evsel, "next_comm", 16, false))
> +	if (evsel__test_field_alt(evsel, "next_comm", TASK_COMM_LEN_16,
> +				  TASK_COMM_LEN, false))
>  		ret = -1;
>  
>  	if (evsel__test_field(evsel, "next_pid", 4, true))
> @@ -72,7 +85,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
>  		return -1;
>  	}
>  
> -	if (evsel__test_field(evsel, "comm", 16, false))
> +	if (evsel__test_field_alt(evsel, "comm", TASK_COMM_LEN_16,
> +				  TASK_COMM_LEN, false))
>  		ret = -1;
>  
>  	if (evsel__test_field(evsel, "pid", 4, true))
> -- 
> 2.17.1

-- 

- Arnaldo

Re: [PATCH v7 08/11] tools/perf/test: make perf test adopt to task comm size change

[Yafang Shao]

On Wed, Nov 17, 2021 at 10:31 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Mon, Nov 01, 2021 at 06:04:16AM +0000, Yafang Shao escreveu:
> > kernel test robot reported a perf-test failure after I extended task comm
> > size from 16 to 24. The failure as follows,
> >
> > 2021-10-13 18:00:46 sudo /usr/src/perf_selftests-x86_64-rhel-8.3-317419b91ef4eff4e2f046088201e4dc4065caa0/tools/perf/perf test 15
> > 15: Parse sched tracepoints fields                                  : FAILED!
> >
> > The reason is perf-test requires a fixed-size task comm. If we extend
> > task comm size to 24, it will not equil with the required size 16 in perf
> > test.
> >
> > After some analyzation, I found perf itself can adopt to the size
> > change, for example, below is the output of perf-sched after I extend
> > comm size to 24 -
> >
> > task    614 (            kthreadd:        84), nr_events: 1
> > task    615 (             systemd:       843), nr_events: 1
> > task    616 (     networkd-dispat:      1026), nr_events: 1
> > task    617 (             systemd:       846), nr_events: 1
> >
> > $ cat /proc/843/comm
> > networkd-dispatcher
> >
> > The task comm can be displayed correctly as expected.
> >
> > Replace old hard-coded 16 with the new one can fix the warning, but we'd
> > better make the test accept both old and new sizes, then it can be
> > backward compatibility.
> >
> > After this patch, the perf-test succeeds no matter task comm is 16 or
> > 24 -
> >
> > 15: Parse sched tracepoints fields                                  : Ok
> >
> > This patch is a preparation for the followup patch.
> >
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Suggested-by: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> > 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: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Petr Mladek <pmladek@suse.com>
> > ---
> >  tools/include/linux/sched.h       | 11 +++++++++++
> >  tools/perf/tests/evsel-tp-sched.c | 26 ++++++++++++++++++++------
> >  2 files changed, 31 insertions(+), 6 deletions(-)
> >  create mode 100644 tools/include/linux/sched.h
> >
> > diff --git a/tools/include/linux/sched.h b/tools/include/linux/sched.h
> > new file mode 100644
> > index 000000000000..0d575afd7f43
> > --- /dev/null
> > +++ b/tools/include/linux/sched.h
> > @@ -0,0 +1,11 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _TOOLS_LINUX_SCHED_H
> > +#define _TOOLS_LINUX_SCHED_H
> > +
> > +/* Keep both length for backward compatibility */
> > +enum {
> > +     TASK_COMM_LEN_16 = 16,
> > +     TASK_COMM_LEN = 24,
> > +};
> > +
>
> I don't think this is a good idea, to have it in tools/include/linux/,
> we have /usr/include/linux/sched.h, this may end up confusing the build
> at some point as your proposal is for a trimmed down header while what
> is in /usr/include/linux/sched.h doesn't have just this.
>
> But since we're using enums for this, we can't check for it with:
>
> #ifdef TASK_COMM_LEN_16
> #define TASK_COMM_LEN_16 16
> #endif
>
> ditto for TASK_COMM_LEN and be future proof, so I'd say just use
> hardcoded values in tools/perf/tests/evsel-tp-sched.c?
>

Hi Arnaldo,

Thanks for the review.
This perf tests code won't be changed in the latest version as we
don't want to extend comm size any more, see also
https://lore.kernel.org/lkml/20211108083840.4627-1-laoar.shao@gmail.com/
The hard-coded 16 in tools/perf/tests/evsel-tp-sched.c is kept as-is.

>
> > +#endif  /* _TOOLS_LINUX_SCHED_H */
> > diff --git a/tools/perf/tests/evsel-tp-sched.c b/tools/perf/tests/evsel-tp-sched.c
> > index f9e34bd26cf3..029f2a8c8e51 100644
> > --- a/tools/perf/tests/evsel-tp-sched.c
> > +++ b/tools/perf/tests/evsel-tp-sched.c
> > @@ -1,11 +1,13 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include <linux/err.h>
> > +#include <linux/sched.h>
> >  #include <traceevent/event-parse.h>
> >  #include "evsel.h"
> >  #include "tests.h"
> >  #include "debug.h"
> >
> > -static int evsel__test_field(struct evsel *evsel, const char *name, int size, bool should_be_signed)
> > +static int evsel__test_field_alt(struct evsel *evsel, const char *name,
> > +                              int size, int alternate_size, bool should_be_signed)
> >  {
> >       struct tep_format_field *field = evsel__field(evsel, name);
> >       int is_signed;
> > @@ -23,15 +25,24 @@ static int evsel__test_field(struct evsel *evsel, const char *name, int size, bo
> >               ret = -1;
> >       }
> >
> > -     if (field->size != size) {
> > -             pr_debug("%s: \"%s\" size (%d) should be %d!\n",
> > +     if (field->size != size && field->size != alternate_size) {
> > +             pr_debug("%s: \"%s\" size (%d) should be %d",
> >                        evsel->name, name, field->size, size);
> > +             if (alternate_size > 0)
> > +                     pr_debug(" or %d", alternate_size);
> > +             pr_debug("!\n");
> >               ret = -1;
> >       }
> >
> >       return ret;
> >  }
> >
> > +static int evsel__test_field(struct evsel *evsel, const char *name,
> > +                          int size, bool should_be_signed)
> > +{
> > +     return evsel__test_field_alt(evsel, name, size, -1, should_be_signed);
> > +}
> > +
> >  int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtest __maybe_unused)
> >  {
> >       struct evsel *evsel = evsel__newtp("sched", "sched_switch");
> > @@ -42,7 +53,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
> >               return -1;
> >       }
> >
> > -     if (evsel__test_field(evsel, "prev_comm", 16, false))
> > +     if (evsel__test_field_alt(evsel, "prev_comm", TASK_COMM_LEN_16,
> > +                               TASK_COMM_LEN, false))
> >               ret = -1;
> >
> >       if (evsel__test_field(evsel, "prev_pid", 4, true))
> > @@ -54,7 +66,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
> >       if (evsel__test_field(evsel, "prev_state", sizeof(long), true))
> >               ret = -1;
> >
> > -     if (evsel__test_field(evsel, "next_comm", 16, false))
> > +     if (evsel__test_field_alt(evsel, "next_comm", TASK_COMM_LEN_16,
> > +                               TASK_COMM_LEN, false))
> >               ret = -1;
> >
> >       if (evsel__test_field(evsel, "next_pid", 4, true))
> > @@ -72,7 +85,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtes
> >               return -1;
> >       }
> >
> > -     if (evsel__test_field(evsel, "comm", 16, false))
> > +     if (evsel__test_field_alt(evsel, "comm", TASK_COMM_LEN_16,
> > +                               TASK_COMM_LEN, false))
> >               ret = -1;
> >
> >       if (evsel__test_field(evsel, "pid", 4, true))
> > --
> > 2.17.1
>
> --
>
> - Arnaldo



-- 
Thanks
Yafang