[PATCH 0/7] task comm cleanups

[PATCH 0/7] task comm cleanups

[Yafang Shao]

This patchset is part of the patchset "extend task comm from 16 to 24"[1].
Now we have different opinion that dynamically allocates memory to store 
kthread's long name into a separate pointer, so I decide to take the useful
cleanups apart from the original patchset and send it separately[2].

These useful cleanups can make the usage around task comm less
error-prone. Furthermore, it will be useful if we want to extend task
comm in the future.

All of the patches except patch #4 have either a reviewed-by or a
acked-by now. I have verfied that the vmcore/crash works well after
patch #4.

[1]. https://lore.kernel.org/lkml/20211101060419.4682-1-laoar.shao@gmail.com/
[2]. https://lore.kernel.org/lkml/CALOAHbAx55AUo3bm8ZepZSZnw7A08cvKPdPyNTf=E_tPqmw5hw@mail.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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: David Hildenbrand <david@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>

Yafang Shao (7):
  fs/exec: make __set_task_comm always set a nul terminated string
  fs/exec: make __get_task_comm always get a nul terminated string
  drivers/infiniband: use get_task_comm instead of open-coded string
    copy
  fs/binfmt_elf: use get_task_comm instead of open-coded string copy
  samples/bpf/test_overhead_kprobe_kern: make it adopt to task comm size
    change
  tools/bpf/bpftool/skeleton: use bpf_probe_read_kernel_str to get task
    comm
  tools/testing/selftests/bpf: make it adopt to task comm size change

 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                                 |  9 +++++++--
 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 ++--
 .../testing/selftests/bpf/progs/test_stacktrace_map.c |  6 +++---
 tools/testing/selftests/bpf/progs/test_tracepoint.c   |  6 +++---
 11 files changed, 32 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH 1/7] 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl> 
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: David Hildenbrand <david@redhat.com>
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 2/7] 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. 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: David Hildenbrand <david@redhat.com>
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 3/7] drivers/infiniband: use get_task_comm instead of open-coded string copy

[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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: David Hildenbrand <david@redhat.com>
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 4/7] fs/binfmt_elf: use get_task_comm instead of open-coded string copy

[Yafang Shao]

It is better to use get_task_comm() instead of the open coded string
copy as we do in other places.

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.

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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: David Hildenbrand <david@redhat.com>
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 5/7] 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: David Hildenbrand <david@redhat.com>
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 6/7] tools/bpf/bpftool/skeleton: use bpf_probe_read_kernel_str to get task comm

[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>
Acked-by: Andrii Nakryiko <andrii@kernel.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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: David Hildenbrand <david@redhat.com>
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 7/7] 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>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: David Hildenbrand <david@redhat.com>
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 e0454e60fe8f..1c456691f890 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

Re: [PATCH 5/7] samples/bpf/test_overhead_kprobe_kern: make it adopt to task comm size change

[Andrii Nakryiko]

On Mon, Nov 8, 2021 at 12:39 AM 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. 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: David Hildenbrand <david@redhat.com>
> 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>

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

[...]

Re: [PATCH 0/7] task comm cleanups

[Kees Cook]

On Mon, Nov 08, 2021 at 08:38:33AM +0000, Yafang Shao wrote:
> This patchset is part of the patchset "extend task comm from 16 to 24"[1].
> Now we have different opinion that dynamically allocates memory to store 
> kthread's long name into a separate pointer, so I decide to take the useful
> cleanups apart from the original patchset and send it separately[2].
> 
> These useful cleanups can make the usage around task comm less
> error-prone. Furthermore, it will be useful if we want to extend task
> comm in the future.
> 
> All of the patches except patch #4 have either a reviewed-by or a
> acked-by now. I have verfied that the vmcore/crash works well after
> patch #4.
> 
> [1]. https://lore.kernel.org/lkml/20211101060419.4682-1-laoar.shao@gmail.com/
> [2]. https://lore.kernel.org/lkml/CALOAHbAx55AUo3bm8ZepZSZnw7A08cvKPdPyNTf=E_tPqmw5hw@mail.gmail.com/

Thanks for collecting this! It all looks good to me.

Andrew, can you take these?

-Kees

-- 
Kees Cook

Re: [PATCH 1/7] fs/exec: make __set_task_comm always set a nul terminated string

[David Hildenbrand]

On 08.11.21 09:38, Yafang Shao wrote:
> Make sure the string set to task comm is always nul terminated.
> 

strlcpy: "the result is always a valid NUL-terminated string that fits
in the buffer"

The only difference seems to be that strscpy_pad() pads the remainder
with zeroes.

Is this description correct and I am missing something important?

> 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: David Hildenbrand <david@redhat.com>
> 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);
>  }
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH 1/7] fs/exec: make __set_task_comm always set a nul terminated string

[Yafang Shao]

On Wed, Nov 10, 2021 at 4:28 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 08.11.21 09:38, Yafang Shao wrote:
> > Make sure the string set to task comm is always nul terminated.
> >
>
> strlcpy: "the result is always a valid NUL-terminated string that fits
> in the buffer"
>
> The only difference seems to be that strscpy_pad() pads the remainder
> with zeroes.
>
> Is this description correct and I am missing something important?
>

In a earlier version [1], the checkpatch.py found a warning:
WARNING: Prefer strscpy over strlcpy - see:
https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
So I replaced strlcpy() with strscpy() to fix this warning.
And then in v5[2], the strscpy() was replaced with strscpy_pad() to
make sure there's no garbade data and also make get_task_comm() be
consistent with get_task_comm().

This commit log didn't clearly describe the historical changes.  So I
think we can update the commit log and subject with:

Subject: use strscpy_pad with strlcpy in __set_task_comm
Commit log:
strlcpy is not suggested to use by the checkpatch.pl, so we'd better
recplace it with strscpy.
To avoid leaving garbage data and be consistent with the usage in
__get_task_comm(), the strscpy_pad is used here.

WDYT?

[1]. https://lore.kernel.org/lkml/20211007120752.5195-3-laoar.shao@gmail.com/
[2]. https://lore.kernel.org/lkml/20211021034516.4400-2-laoar.shao@gmail.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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: David Hildenbrand <david@redhat.com>
> > 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);
> >  }
> >
>
>
> --
> Thanks,
>
> David / dhildenb
>


-- 
Thanks
Yafang

Re: [PATCH 1/7] fs/exec: make __set_task_comm always set a nul terminated string

[Kees Cook]

On Wed, Nov 10, 2021 at 09:28:12AM +0100, David Hildenbrand wrote:
> On 08.11.21 09:38, Yafang Shao wrote:
> > Make sure the string set to task comm is always nul terminated.
> > 
> 
> strlcpy: "the result is always a valid NUL-terminated string that fits
> in the buffer"
> 
> The only difference seems to be that strscpy_pad() pads the remainder
> with zeroes.
> 
> Is this description correct and I am missing something important?

Yes, this makes sure it's zero padded just to be robust against full
tsk->comm copies that got noticed in other places.

The only other change is that we want to remove strlcpy() from the
kernel generally since it can trigger out-of-bound reads on the source
string[1].

So, in this case, the most robust version is to use strscpy_pad().

-Kees

[1] https://github.com/KSPP/linux/issues/89

> 
> > 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl> 
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: David Hildenbrand <david@redhat.com>
> > 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);
> >  }
> > 
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb
> 

-- 
Kees Cook

Re: [PATCH 1/7] fs/exec: make __set_task_comm always set a nul terminated string

[David Hildenbrand]

On 10.11.21 10:05, Yafang Shao wrote:
> On Wed, Nov 10, 2021 at 4:28 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 08.11.21 09:38, Yafang Shao wrote:
>>> Make sure the string set to task comm is always nul terminated.
>>>
>>
>> strlcpy: "the result is always a valid NUL-terminated string that fits
>> in the buffer"
>>
>> The only difference seems to be that strscpy_pad() pads the remainder
>> with zeroes.
>>
>> Is this description correct and I am missing something important?
>>
> 
> In a earlier version [1], the checkpatch.py found a warning:
> WARNING: Prefer strscpy over strlcpy - see:
> https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
> So I replaced strlcpy() with strscpy() to fix this warning.
> And then in v5[2], the strscpy() was replaced with strscpy_pad() to
> make sure there's no garbade data and also make get_task_comm() be
> consistent with get_task_comm().
> 
> This commit log didn't clearly describe the historical changes.  So I
> think we can update the commit log and subject with:
> 
> Subject: use strscpy_pad with strlcpy in __set_task_comm
> Commit log:
> strlcpy is not suggested to use by the checkpatch.pl, so we'd better
> recplace it with strscpy.
> To avoid leaving garbage data and be consistent with the usage in
> __get_task_comm(), the strscpy_pad is used here.
> 
> WDYT?

Yes, that makes it clearer what this patch actually does :)

With the subject+description changed

Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/7] fs/exec: make __get_task_comm always get a nul terminated string

[David Hildenbrand]

On 08.11.21 09:38, Yafang Shao wrote:
> If the dest buffer size is smaller than sizeof(tsk->comm), the buffer
> will be without null ternimator, that may cause problem. 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Petr Mladek <pmladek@suse.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH 3/7] drivers/infiniband: use get_task_comm instead of open-coded string copy

[David Hildenbrand]

On 08.11.21 09:38, Yafang Shao wrote:
> 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Petr Mladek <pmladek@suse.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH 4/7] fs/binfmt_elf: use get_task_comm instead of open-coded string copy

[David Hildenbrand]

On 08.11.21 09:38, Yafang Shao wrote:
> It is better to use get_task_comm() instead of the open coded string
> copy as we do in other places.
> 
> 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.
> 
> 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: David Hildenbrand <david@redhat.com>
> 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;
>  }
> 

We have a hard-coded "pr_fname[16]" as well, not sure if we want to
adjust that to use TASK_COMM_LEN?

Anyhow

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH 4/7] fs/binfmt_elf: use get_task_comm instead of open-coded string copy

[David Hildenbrand]

On 11.11.21 11:03, David Hildenbrand wrote:
> On 08.11.21 09:38, Yafang Shao wrote:
>> It is better to use get_task_comm() instead of the open coded string
>> copy as we do in other places.
>>
>> 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.
>>
>> 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Matthew Wilcox <willy@infradead.org>
>> Cc: David Hildenbrand <david@redhat.com>
>> 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;
>>  }
>>
> 
> We have a hard-coded "pr_fname[16]" as well, not sure if we want to
> adjust that to use TASK_COMM_LEN?

But if the intention is to chance TASK_COMM_LEN later, we might want to
keep that unchanged.

(replacing the 16 by a define might still be a good idea, similar to how
it's done for ELF_PRARGSZ, but just a thought)


-- 
Thanks,

David / dhildenb

Re: [PATCH 5/7] samples/bpf/test_overhead_kprobe_kern: make it adopt to task comm size change

[David Hildenbrand]

On 08.11.21 09:38, Yafang Shao 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. 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: David Hildenbrand <david@redhat.com>
> 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));

It's a shame we have to do a manual copy here ...

Changes LGTM

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 6/7] tools/bpf/bpftool/skeleton: use bpf_probe_read_kernel_str to get task comm

[David Hildenbrand]

On 08.11.21 09:38, Yafang Shao 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>
> Acked-by: Andrii Nakryiko <andrii@kernel.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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Petr Mladek <pmladek@suse.com>

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 08.11.21 09:38, Yafang Shao 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>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Petr Mladek <pmladek@suse.com>


Acked-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH 4/7] fs/binfmt_elf: use get_task_comm instead of open-coded string copy

[Petr Mladek]

On Thu 2021-11-11 11:06:04, David Hildenbrand wrote:
> On 11.11.21 11:03, David Hildenbrand wrote:
> > On 08.11.21 09:38, Yafang Shao wrote:
> >> It is better to use get_task_comm() instead of the open coded string
> >> copy as we do in other places.
> >>
> >> 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.
> >>
> >> 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >> Cc: Steven Rostedt <rostedt@goodmis.org>
> >> Cc: Matthew Wilcox <willy@infradead.org>
> >> Cc: David Hildenbrand <david@redhat.com>
> >> 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;
> >>  }
> >>
> > 
> > We have a hard-coded "pr_fname[16]" as well, not sure if we want to
> > adjust that to use TASK_COMM_LEN?
> 
> But if the intention is to chance TASK_COMM_LEN later, we might want to
> keep that unchanged.

It seems that len will not change in the end. Another solution is
going to be used for the long names, see
https://lore.kernel.org/r/20211108084142.4692-1-laoar.shao@gmail.com.

> (replacing the 16 by a define might still be a good idea, similar to how
> it's done for ELF_PRARGSZ, but just a thought)

If the code would need some tweaking when the size changes, you could
still use TASK_COMM_LEN and trigger a compilation error when the size
gets modified. For example, static_assert(TASK_COMM_LEN == 16);

It will make it clear that it needs attention if the size is ever modified.

Best Regards,
Petr

Re: [PATCH 4/7] fs/binfmt_elf: use get_task_comm instead of open-coded string copy

[David Hildenbrand]

On 11.11.21 12:34, Petr Mladek wrote:
> On Thu 2021-11-11 11:06:04, David Hildenbrand wrote:
>> On 11.11.21 11:03, David Hildenbrand wrote:
>>> On 08.11.21 09:38, Yafang Shao wrote:
>>>> It is better to use get_task_comm() instead of the open coded string
>>>> copy as we do in other places.
>>>>
>>>> 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.
>>>>
>>>> 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>> Cc: Steven Rostedt <rostedt@goodmis.org>
>>>> Cc: Matthew Wilcox <willy@infradead.org>
>>>> Cc: David Hildenbrand <david@redhat.com>
>>>> 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;
>>>>  }
>>>>
>>>
>>> We have a hard-coded "pr_fname[16]" as well, not sure if we want to
>>> adjust that to use TASK_COMM_LEN?
>>
>> But if the intention is to chance TASK_COMM_LEN later, we might want to
>> keep that unchanged.
> 
> It seems that len will not change in the end. Another solution is
> going to be used for the long names, see
> https://lore.kernel.org/r/20211108084142.4692-1-laoar.shao@gmail.com.

Yes, that's what I recall as well. The I read the patch
subjects+descriptions in this series "make it adopt to task comm size
change" and was slightly confused.

Maybe we should just remove any notion of "task comm size change" from
this series and instead just call it a cleanup.


-- 
Thanks,

David / dhildenb

Re: [PATCH 4/7] fs/binfmt_elf: use get_task_comm instead of open-coded string copy

[Yafang Shao]

On Thu, Nov 11, 2021 at 7:35 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Thu 2021-11-11 11:06:04, David Hildenbrand wrote:
> > On 11.11.21 11:03, David Hildenbrand wrote:
> > > On 08.11.21 09:38, Yafang Shao wrote:
> > >> It is better to use get_task_comm() instead of the open coded string
> > >> copy as we do in other places.
> > >>
> > >> 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.
> > >>
> > >> 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
> > >> Cc: Peter Zijlstra <peterz@infradead.org>
> > >> Cc: Steven Rostedt <rostedt@goodmis.org>
> > >> Cc: Matthew Wilcox <willy@infradead.org>
> > >> Cc: David Hildenbrand <david@redhat.com>
> > >> 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;
> > >>  }
> > >>
> > >
> > > We have a hard-coded "pr_fname[16]" as well, not sure if we want to
> > > adjust that to use TASK_COMM_LEN?
> >
> > But if the intention is to chance TASK_COMM_LEN later, we might want to
> > keep that unchanged.
>
> It seems that len will not change in the end. Another solution is
> going to be used for the long names, see
> https://lore.kernel.org/r/20211108084142.4692-1-laoar.shao@gmail.com.
>
> > (replacing the 16 by a define might still be a good idea, similar to how
> > it's done for ELF_PRARGSZ, but just a thought)
>
> If the code would need some tweaking when the size changes, you could
> still use TASK_COMM_LEN and trigger a compilation error when the size
> gets modified. For example, static_assert(TASK_COMM_LEN == 16);
>
> It will make it clear that it needs attention if the size is ever modified.
>

I think we can just add some comments to make it grepable, for example,

+  /* TASK_COMM_LEN */
    char pr_fname[16];

or a more detailed explanation:

+  /*
+   * The hard-coded 16 is derived from TASK_COMM_LEN, but it can be changed as
+   *  it is exposed to userspace. We'd better make it hard-coded here.
+   */
char pr_fname[16];

-- 
Thanks
Yafang

Re: [PATCH 4/7] fs/binfmt_elf: use get_task_comm instead of open-coded string copy

[Yafang Shao]

On Thu, Nov 11, 2021 at 7:47 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 11.11.21 12:34, Petr Mladek wrote:
> > On Thu 2021-11-11 11:06:04, David Hildenbrand wrote:
> >> On 11.11.21 11:03, David Hildenbrand wrote:
> >>> On 08.11.21 09:38, Yafang Shao wrote:
> >>>> It is better to use get_task_comm() instead of the open coded string
> >>>> copy as we do in other places.
> >>>>
> >>>> 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.
> >>>>
> >>>> 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: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
> >>>> Cc: Peter Zijlstra <peterz@infradead.org>
> >>>> Cc: Steven Rostedt <rostedt@goodmis.org>
> >>>> Cc: Matthew Wilcox <willy@infradead.org>
> >>>> Cc: David Hildenbrand <david@redhat.com>
> >>>> 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;
> >>>>  }
> >>>>
> >>>
> >>> We have a hard-coded "pr_fname[16]" as well, not sure if we want to
> >>> adjust that to use TASK_COMM_LEN?
> >>
> >> But if the intention is to chance TASK_COMM_LEN later, we might want to
> >> keep that unchanged.
> >
> > It seems that len will not change in the end. Another solution is
> > going to be used for the long names, see
> > https://lore.kernel.org/r/20211108084142.4692-1-laoar.shao@gmail.com.
>
> Yes, that's what I recall as well. The I read the patch
> subjects+descriptions in this series "make it adopt to task comm size
> change" and was slightly confused.
>
> Maybe we should just remove any notion of "task comm size change" from
> this series and instead just call it a cleanup.
>
>

Agreed that it may make a little confused for the one who didn't
engage in this series at the first place.
I will improve the subjection+description.


-- 
Thanks
Yafang