[PATCH 0/7] task comm cleanups

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 <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

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

Signed-off-by: Yafang Shao <redacted>
Reviewed-by: Kees Cook <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

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 <redacted>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Yafang Shao <redacted>
Reviewed-by: Kees Cook <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

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 <redacted>
Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

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 <redacted>
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

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 <redacted>
Reviewed-by: Kees Cook <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

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 <redacted>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

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 <redacted>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/sched.h                                   | 9 +++++++--
 tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 6 +++---
 tools/testing/selftests/bpf/progs/test_tracepoint.c     | 6 +++---
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 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

On Mon, Nov 8, 2021 at 12:39 AM Yafang Shao [off-list ref] 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 <redacted>
Reviewed-by: Kees Cook <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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(-)

[...]

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

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?

-- 
Thanks,

David / dhildenb

On Wed, Nov 10, 2021 at 4:28 PM David Hildenbrand [off-list ref] wrote:

On 08.11.21 09:38, Yafang Shao wrote:

quoted

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/

quoted

Signed-off-by: Yafang Shao <redacted>
Reviewed-by: Kees Cook <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

On Wed, Nov 10, 2021 at 09:28:12AM +0100, David Hildenbrand wrote:

On 08.11.21 09:38, Yafang Shao wrote:

quoted

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

quoted

Signed-off-by: Yafang Shao <redacted>
Reviewed-by: Kees Cook <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

On 10.11.21 10:05, Yafang Shao wrote:

On Wed, Nov 10, 2021 at 4:28 PM David Hildenbrand [off-list ref] wrote:

quoted

On 08.11.21 09:38, Yafang Shao wrote:

quoted

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

Thanks!

-- 
Thanks,

David / dhildenb

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 <redacted>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Yafang Shao <redacted>
Reviewed-by: Kees Cook <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---

Reviewed-by: David Hildenbrand <redacted>


-- 
Thanks,

David / dhildenb

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 <redacted>
Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---

Reviewed-by: David Hildenbrand <redacted>


-- 
Thanks,

David / dhildenb

On 08.11.21 09:38, Yafang Shao wrote:

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


-- 
Thanks,

David / dhildenb

On 11.11.21 11:03, David Hildenbrand wrote:

On 08.11.21 09:38, Yafang Shao wrote:

quoted

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]

quoted

    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]

quoted

    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 <redacted>
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

On 08.11.21 09:38, Yafang Shao wrote:

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

Changes LGTM

Reviewed-by: David Hildenbrand <redacted>

-- 
Thanks,

David / dhildenb

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 <redacted>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>

Reviewed-by: David Hildenbrand <redacted>


-- 
Thanks,

David / dhildenb

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 <redacted>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>

Acked-by: David Hildenbrand <redacted>


-- 
Thanks,

David / dhildenb

On Thu 2021-11-11 11:06:04, David Hildenbrand wrote:

On 11.11.21 11:03, David Hildenbrand wrote:

quoted

On 08.11.21 09:38, Yafang Shao wrote:

quoted

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]

quoted

    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]

quoted

    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 <redacted>
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

On 11.11.21 12:34, Petr Mladek wrote:

On Thu 2021-11-11 11:06:04, David Hildenbrand wrote:

quoted

On 11.11.21 11:03, David Hildenbrand wrote:

quoted

On 08.11.21 09:38, Yafang Shao wrote:

quoted

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]

quoted

    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]

quoted

    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 <redacted>
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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

On Thu, Nov 11, 2021 at 7:35 PM Petr Mladek [off-list ref] wrote:

On Thu 2021-11-11 11:06:04, David Hildenbrand wrote:

quoted

On 11.11.21 11:03, David Hildenbrand wrote:

quoted

On 08.11.21 09:38, Yafang Shao wrote:

quoted

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]

quoted

    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]

quoted

    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 <redacted>
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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.

quoted

(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

On Thu, Nov 11, 2021 at 7:47 PM David Hildenbrand [off-list ref] wrote:

On 11.11.21 12:34, Petr Mladek wrote:

quoted

On Thu 2021-11-11 11:06:04, David Hildenbrand wrote:

quoted

On 11.11.21 11:03, David Hildenbrand wrote:

quoted

On 08.11.21 09:38, Yafang Shao wrote:

quoted

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]

quoted

    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]

quoted

    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 <redacted>
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
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 <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
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