[PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL

[PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL

[Yafang Shao]

This patchset changes files among many subsystems. I don't know which
tree it should be applied to, so I just base it on Linus's tree.

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

This patchset tries to improve this problem fundamentally by extending
the task comm size from 16 to 24. In order to do that, we have to do
some cleanups first.

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

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

2. Replace the old hard-coded 16 with a new macro TASK_COMM_LEN_16 to
make it more grepable.

3. Extend the task comm size to 24 for CONFIG_BASE_FULL case and keep it
as 16 for CONFIG_BASE_SMALL.

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

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

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

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

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

Yafang Shao (15):
  fs/exec: make __set_task_comm always set a nul ternimated string
  fs/exec: make __get_task_comm always get a nul terminated string
  sched.h: introduce TASK_COMM_LEN_16
  cn_proc: make connector comm always nul ternimated
  drivers/infiniband: make setup_ctxt always get a nul terminated task
    comm
  elfcore: make prpsinfo always get a nul terminated task comm
  samples/bpf/kern: use TASK_COMM_LEN instead of hard-coded 16
  samples/bpf/user: use TASK_COMM_LEN_16 instead of hard-coded 16
  tools/include: introduce TASK_COMM_LEN_16
  tools/lib/perf: use TASK_COMM_LEN_16 instead of hard-coded 16
  tools/bpf/bpftool: use TASK_COMM_LEN_16 instead of hard-coded 16
  tools/perf/test: make perf test adopt to task comm size change
  tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of
    hard-coded 16
  sched.h: extend task comm from 16 to 24 for CONFIG_BASE_FULL
  kernel/kthread: show a warning if kthread's comm is truncated

 drivers/connector/cn_proc.c                   |  5 +++-
 drivers/infiniband/hw/qib/qib.h               |  4 +--
 drivers/infiniband/hw/qib/qib_file_ops.c      |  2 +-
 fs/binfmt_elf.c                               |  2 +-
 fs/exec.c                                     |  5 ++--
 include/linux/elfcore-compat.h                |  3 ++-
 include/linux/elfcore.h                       |  4 +--
 include/linux/sched.h                         | 11 +++++++-
 include/uapi/linux/cn_proc.h                  |  7 ++++-
 kernel/kthread.c                              |  7 ++++-
 samples/bpf/offwaketime_kern.c                | 10 +++----
 samples/bpf/offwaketime_user.c                |  6 ++---
 samples/bpf/test_overhead_kprobe_kern.c       | 11 ++++----
 samples/bpf/test_overhead_tp_kern.c           |  5 ++--
 samples/bpf/tracex2_kern.c                    |  3 ++-
 samples/bpf/tracex2_user.c                    |  7 ++---
 tools/bpf/bpftool/Makefile                    |  1 +
 tools/bpf/bpftool/main.h                      |  3 ++-
 tools/bpf/bpftool/skeleton/pid_iter.bpf.c     |  4 +--
 tools/bpf/bpftool/skeleton/pid_iter.h         |  4 ++-
 tools/include/linux/sched/task.h              |  3 +++
 tools/lib/perf/include/perf/event.h           |  5 ++--
 tools/perf/tests/evsel-tp-sched.c             | 26 ++++++++++++++-----
 tools/testing/selftests/bpf/Makefile          |  2 +-
 .../selftests/bpf/prog_tests/ringbuf.c        |  3 ++-
 .../selftests/bpf/prog_tests/ringbuf_multi.c  |  3 ++-
 .../bpf/prog_tests/sk_storage_tracing.c       |  3 ++-
 .../selftests/bpf/prog_tests/test_overhead.c  |  3 ++-
 .../bpf/prog_tests/trampoline_count.c         |  3 ++-
 tools/testing/selftests/bpf/progs/profiler.h  |  7 ++---
 .../selftests/bpf/progs/profiler.inc.h        |  8 +++---
 tools/testing/selftests/bpf/progs/pyperf.h    |  4 +--
 .../testing/selftests/bpf/progs/strobemeta.h  |  6 ++---
 .../bpf/progs/test_core_reloc_kernel.c        |  3 ++-
 .../selftests/bpf/progs/test_ringbuf.c        |  3 ++-
 .../selftests/bpf/progs/test_ringbuf_multi.c  |  3 ++-
 .../bpf/progs/test_sk_storage_tracing.c       |  5 ++--
 .../selftests/bpf/progs/test_skb_helpers.c    |  5 ++--
 .../selftests/bpf/progs/test_stacktrace_map.c |  5 ++--
 .../selftests/bpf/progs/test_tracepoint.c     |  5 ++--
 40 files changed, 135 insertions(+), 74 deletions(-)

-- 
2.17.1

[PATCH v5 01/15] fs/exec: make __set_task_comm always set a nul ternimated string

[Yafang Shao]

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

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 fs/exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[Yafang Shao]

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

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

Suggested-by: Kees Cook <keescook@chromium.org>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
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..bf2a7a91eeea 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);
+	/* The copied value is always null terminated */
+	strscpy_pad(buf, tsk->comm, buf_size);
 	task_unlock(tsk);
 	return buf;
 }
-- 
2.17.1

[PATCH v5 03/15] sched.h: introduce TASK_COMM_LEN_16

[Yafang Shao]

There're many hard-coded 16 used to store task comm in the kernel, that
makes it error prone if we want to change the value of TASK_COMM_LEN. A
new marco TASK_COMM_LEN_16 is introduced to replace these old ones, then
we can easily grep them.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/sched.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index c1a927ddec64..62d5b30d310c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -274,6 +274,8 @@ struct task_group;
 
 #define get_current_state()	READ_ONCE(current->__state)
 
+/* To replace the old hard-coded 16 */
+#define TASK_COMM_LEN_16		16
 /* Task command name length: */
 #define TASK_COMM_LEN			16
 
-- 
2.17.1

Re: [PATCH v5 03/15] sched.h: introduce TASK_COMM_LEN_16

[Andrii Nakryiko]

On Wed, Oct 20, 2021 at 8:45 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> There're many hard-coded 16 used to store task comm in the kernel, that
> makes it error prone if we want to change the value of TASK_COMM_LEN. A
> new marco TASK_COMM_LEN_16 is introduced to replace these old ones, then
> we can easily grep them.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Petr Mladek <pmladek@suse.com>
> ---
>  include/linux/sched.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index c1a927ddec64..62d5b30d310c 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -274,6 +274,8 @@ struct task_group;
>
>  #define get_current_state()    READ_ONCE(current->__state)
>
> +/* To replace the old hard-coded 16 */
> +#define TASK_COMM_LEN_16               16
>  /* Task command name length: */
>  #define TASK_COMM_LEN                  16

Can we please convert these two constants into enum? That will allow
BPF applications to deal with such kernel change more easily because
these constants will now be available as part of kernel BTF.

Something like this should be completely equivalent for all the kernel uses:

enum {
    TASK_COMM_LEN = 16,
    TASK_COMM_LEN_16 = 16,
};

When later TASK_COMM_LEN is defined as = 24, BPF applications will be
able to deal with that by querying BTF through BPF CO-RE.

>
> --
> 2.17.1
>

Re: [PATCH v5 03/15] sched.h: introduce TASK_COMM_LEN_16

[Yafang Shao]

On Fri, Oct 22, 2021 at 5:55 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Oct 20, 2021 at 8:45 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > There're many hard-coded 16 used to store task comm in the kernel, that
> > makes it error prone if we want to change the value of TASK_COMM_LEN. A
> > new marco TASK_COMM_LEN_16 is introduced to replace these old ones, then
> > we can easily grep them.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Petr Mladek <pmladek@suse.com>
> > ---
> >  include/linux/sched.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index c1a927ddec64..62d5b30d310c 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -274,6 +274,8 @@ struct task_group;
> >
> >  #define get_current_state()    READ_ONCE(current->__state)
> >
> > +/* To replace the old hard-coded 16 */
> > +#define TASK_COMM_LEN_16               16
> >  /* Task command name length: */
> >  #define TASK_COMM_LEN                  16
>
> Can we please convert these two constants into enum? That will allow
> BPF applications to deal with such kernel change more easily because
> these constants will now be available as part of kernel BTF.
>
> Something like this should be completely equivalent for all the kernel uses:
>
> enum {
>     TASK_COMM_LEN = 16,
>     TASK_COMM_LEN_16 = 16,
> };
>
> When later TASK_COMM_LEN is defined as = 24, BPF applications will be
> able to deal with that by querying BTF through BPF CO-RE.
>

Sure. I will convert it to enum.


-- 
Thanks
Yafang

[PATCH v5 04/15] cn_proc: make connector comm always nul ternimated

[Yafang Shao]

connector comm was introduced in commit
f786ecba4158 ("connector: add comm change event report to proc connector").
struct comm_proc_event was defined in include/linux/cn_proc.h first and
then been moved into file include/uapi/linux/cn_proc.h in commit
607ca46e97a1 ("UAPI: (Scripted) Disintegrate include/linux").

As this is the UAPI code, we can't change it without potentially breaking
things (i.e. userspace binaries have this size built in, so we can't just
change the size). To prepare for the followup change - extending task
comm, we have to use __get_task_comm() to avoid the BUILD_BUG_ON() in
proc_comm_connector().

__get_task_comm() always get a nul terminated string, so we don't worry
about whether it is truncated or not.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Vladimir Zapolskiy <vzapolskiy@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
 drivers/connector/cn_proc.c  | 5 ++++-
 include/uapi/linux/cn_proc.h | 7 ++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 646ad385e490..c88ba2dc1eae 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -230,7 +230,10 @@ void proc_comm_connector(struct task_struct *task)
 	ev->what = PROC_EVENT_COMM;
 	ev->event_data.comm.process_pid  = task->pid;
 	ev->event_data.comm.process_tgid = task->tgid;
-	get_task_comm(ev->event_data.comm.comm, task);
+
+	/* This may get truncated. */
+	__get_task_comm(ev->event_data.comm.comm,
+			sizeof(ev->event_data.comm.comm), task);
 
 	memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
 	msg->ack = 0; /* not used */
diff --git a/include/uapi/linux/cn_proc.h b/include/uapi/linux/cn_proc.h
index db210625cee8..4bb7f658fcc0 100644
--- a/include/uapi/linux/cn_proc.h
+++ b/include/uapi/linux/cn_proc.h
@@ -21,6 +21,11 @@
 
 #include <linux/types.h>
 
+/* We can't include <linux/sched.h> directly in this UAPI header. */
+#ifndef TASK_COMM_LEN_16
+#define TASK_COMM_LEN_16 16
+#endif
+
 /*
  * Userspace sends this enum to register with the kernel that it is listening
  * for events on the connector.
@@ -110,7 +115,7 @@ struct proc_event {
 		struct comm_proc_event {
 			__kernel_pid_t process_pid;
 			__kernel_pid_t process_tgid;
-			char           comm[16];
+			char           comm[TASK_COMM_LEN_16];
 		} comm;
 
 		struct coredump_proc_event {
-- 
2.17.1

[PATCH v5 05/15] drivers/infiniband: make setup_ctxt always get a nul terminated task comm

[Yafang Shao]

Use TASK_COMM_LEN_16 instead of hard-coded 16 to make it more grepable,
and use strscpy_pad() instead of strlcpy() to make the comm always nul
terminated.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
 drivers/infiniband/hw/qib/qib.h          | 4 ++--
 drivers/infiniband/hw/qib/qib_file_ops.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib.h b/drivers/infiniband/hw/qib/qib.h
index 9363bccfc6e7..8e59f9cbabc8 100644
--- a/drivers/infiniband/hw/qib/qib.h
+++ b/drivers/infiniband/hw/qib/qib.h
@@ -195,8 +195,8 @@ struct qib_ctxtdata {
 	/* pid of process using this ctxt */
 	pid_t pid;
 	pid_t subpid[QLOGIC_IB_MAX_SUBCTXT];
-	/* same size as task_struct .comm[], command that opened context */
-	char comm[16];
+	/* task_struct .comm[], command that opened context */
+	char comm[TASK_COMM_LEN_16];
 	/* 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..7ab2b448c183 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));
+	strscpy_pad(rcd->comm, current->comm, sizeof(rcd->comm));
 	ctxt_fp(fp) = rcd;
 	qib_stats.sps_ctxts++;
 	dd->freectxts--;
-- 
2.17.1

[PATCH v5 06/15] elfcore: make prpsinfo always get a nul terminated task comm

[Yafang Shao]

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

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

This patch can fix this warning.

struct elf_prpsinfo is used to dump the task information in userspace
coredump or kernel vmcore. Use TASK_COMM_LEN_16 instead of hard-coded 16
to make it more grepable, and use strscpy_pad() instead of strncpy() to
make it always get a nul terminated task comm.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
 fs/binfmt_elf.c                | 2 +-
 include/linux/elfcore-compat.h | 3 ++-
 include/linux/elfcore.h        | 4 ++--
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index a813b70f594e..a4ba79fce2a9 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));
+	strscpy_pad(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
 
 	return 0;
 }
diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h
index e272c3d452ce..69fa1a728964 100644
--- a/include/linux/elfcore-compat.h
+++ b/include/linux/elfcore-compat.h
@@ -5,6 +5,7 @@
 #include <linux/elf.h>
 #include <linux/elfcore.h>
 #include <linux/compat.h>
+#include <linux/sched.h>
 
 /*
  * Make sure these layouts match the linux/elfcore.h native definitions.
@@ -43,7 +44,7 @@ struct compat_elf_prpsinfo
 	__compat_uid_t			pr_uid;
 	__compat_gid_t			pr_gid;
 	compat_pid_t			pr_pid, pr_ppid, pr_pgrp, pr_sid;
-	char				pr_fname[16];
+	char				pr_fname[TASK_COMM_LEN_16];
 	char				pr_psargs[ELF_PRARGSZ];
 };
 
diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h
index 2aaa15779d50..ee7ac09734ba 100644
--- a/include/linux/elfcore.h
+++ b/include/linux/elfcore.h
@@ -65,8 +65,8 @@ struct elf_prpsinfo
 	__kernel_gid_t	pr_gid;
 	pid_t	pr_pid, pr_ppid, pr_pgrp, pr_sid;
 	/* Lots missing */
-	char	pr_fname[16];	/* filename of executable */
-	char	pr_psargs[ELF_PRARGSZ];	/* initial part of arg list */
+	char	pr_fname[TASK_COMM_LEN_16];	/* filename of executable */
+	char	pr_psargs[ELF_PRARGSZ];		/* initial part of arg list */
 };
 
 static inline void elf_core_copy_regs(elf_gregset_t *elfregs, struct pt_regs *regs)
-- 
2.17.1

[PATCH v5 07/15] samples/bpf/kern: use TASK_COMM_LEN instead of hard-coded 16

[Yafang Shao]

The linux/sched.h is visible to the bpf kernel modules, so we can use
TASK_COMM_LEN_16 to replace the hard-coded 16 in these bpf kernel
modules to make it more grepable.

In these bpf modules, someone gets task comm via bpf_get_current_comm(),
which always get a nul terminated string. While someone gets task comm via
bpf_probe_read_kernel(), which is unsafe, we should use
bpf_probe_read_kernel_str() instead.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
 samples/bpf/offwaketime_kern.c          | 10 +++++-----
 samples/bpf/test_overhead_kprobe_kern.c | 11 ++++++-----
 samples/bpf/test_overhead_tp_kern.c     |  5 +++--
 samples/bpf/tracex2_kern.c              |  3 ++-
 4 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/samples/bpf/offwaketime_kern.c b/samples/bpf/offwaketime_kern.c
index 4866afd054da..c0fd04497eea 100644
--- a/samples/bpf/offwaketime_kern.c
+++ b/samples/bpf/offwaketime_kern.c
@@ -23,8 +23,8 @@
 #define MAX_ENTRIES	10000
 
 struct key_t {
-	char waker[TASK_COMM_LEN];
-	char target[TASK_COMM_LEN];
+	char waker[TASK_COMM_LEN_16];
+	char target[TASK_COMM_LEN_16];
 	u32 wret;
 	u32 tret;
 };
@@ -44,7 +44,7 @@ struct {
 } start SEC(".maps");
 
 struct wokeby_t {
-	char name[TASK_COMM_LEN];
+	char name[TASK_COMM_LEN_16];
 	u32 ret;
 };
 
@@ -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_16];
 	int prev_pid;
 	int prev_prio;
 	long long prev_state;
-	char next_comm[16];
+	char next_comm[TASK_COMM_LEN_16];
 	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..31e8c5ee0cdc 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_16] = {};
+	char newcomm[TASK_COMM_LEN_16] = {};
 	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..a6d5b3301af2 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_16];
+	char newcomm[TASK_COMM_LEN_16];
 	__u16 oom_score_adj;
 };
 SEC("tracepoint/task/task_rename")
diff --git a/samples/bpf/tracex2_kern.c b/samples/bpf/tracex2_kern.c
index 5bc696bac27d..d70ce59055cb 100644
--- a/samples/bpf/tracex2_kern.c
+++ b/samples/bpf/tracex2_kern.c
@@ -7,6 +7,7 @@
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
 #include <linux/version.h>
+#include <linux/sched.h>
 #include <uapi/linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
@@ -65,7 +66,7 @@ static unsigned int log2l(unsigned long v)
 }
 
 struct hist_key {
-	char comm[16];
+	char comm[TASK_COMM_LEN_16];
 	u64 pid_tgid;
 	u64 uid_gid;
 	u64 index;
-- 
2.17.1

[PATCH v5 08/15] samples/bpf/user: use TASK_COMM_LEN_16 instead of hard-coded 16

[Yafang Shao]

The task comm size is invisible to the bpf userspace, we have to
define a new TASK_COMM_LEN_16 in the userspace. Use this macro instead
of the hard-coded 16 can make it more grepable.

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: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
 samples/bpf/offwaketime_user.c | 6 +++---
 samples/bpf/tracex2_user.c     | 7 ++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/samples/bpf/offwaketime_user.c b/samples/bpf/offwaketime_user.c
index 73a986876c1a..ca918ac93ee7 100644
--- a/samples/bpf/offwaketime_user.c
+++ b/samples/bpf/offwaketime_user.c
@@ -36,11 +36,11 @@ static void print_ksym(__u64 addr)
 		printf("%s;", sym->name);
 }
 
-#define TASK_COMM_LEN 16
+#define TASK_COMM_LEN_16 16
 
 struct key_t {
-	char waker[TASK_COMM_LEN];
-	char target[TASK_COMM_LEN];
+	char waker[TASK_COMM_LEN_16];
+	char target[TASK_COMM_LEN_16];
 	__u32 wret;
 	__u32 tret;
 };
diff --git a/samples/bpf/tracex2_user.c b/samples/bpf/tracex2_user.c
index 1626d51dfffd..70081d917c6d 100644
--- a/samples/bpf/tracex2_user.c
+++ b/samples/bpf/tracex2_user.c
@@ -10,8 +10,9 @@
 #include <bpf/libbpf.h>
 #include "bpf_util.h"
 
-#define MAX_INDEX	64
-#define MAX_STARS	38
+#define MAX_INDEX		64
+#define MAX_STARS		38
+#define TASK_COMM_LEN_16	16
 
 /* my_map, my_hist_map */
 static int map_fd[2];
@@ -28,7 +29,7 @@ static void stars(char *str, long val, long max, int width)
 }
 
 struct task {
-	char comm[16];
+	char comm[TASK_COMM_LEN_16];
 	__u64 pid_tgid;
 	__u64 uid_gid;
 };
-- 
2.17.1

[PATCH v5 09/15] tools/include: introduce TASK_COMM_LEN_16

[Yafang Shao]

TASK_COMM_LEN_16 is introduced to replace all the hard-coded 16 used in
the files under tools/ directory.

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: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
 tools/include/linux/sched/task.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/include/linux/sched/task.h b/tools/include/linux/sched/task.h
index a97890eca110..7657dd3e0e02 100644
--- a/tools/include/linux/sched/task.h
+++ b/tools/include/linux/sched/task.h
@@ -1,4 +1,6 @@
 #ifndef _TOOLS_PERF_LINUX_SCHED_TASK_H
 #define _TOOLS_PERF_LINUX_SCHED_TASK_H
 
+#define TASK_COMM_LEN_16 16
+
 #endif  /* _TOOLS_PERF_LINUX_SCHED_TASK_H */
-- 
2.17.1

Re: [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL

[Andrew Morton]

On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao <laoar.shao@gmail.com> wrote:

> This patchset changes files among many subsystems. I don't know which
> tree it should be applied to, so I just base it on Linus's tree.

I can do that ;)

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

That sucked of us.

> This patchset tries to improve this problem fundamentally by extending
> the task comm size from 16 to 24. In order to do that, we have to do
> some cleanups first.

It's at v5 and there's no evidence of review activity?  C'mon, folks!

> 1. Make the copy of task comm always safe no matter what the task
> comm size is. For example,
> 
>   Unsafe                 Safe
>   strlcpy                strscpy_pad
>   strncpy                strscpy_pad
>   bpf_probe_read_kernel  bpf_probe_read_kernel_str
>                          bpf_core_read_str
>                          bpf_get_current_comm
>                          perf_event__prepare_comm
>                          prctl(2)
> 
> 2. Replace the old hard-coded 16 with a new macro TASK_COMM_LEN_16 to
> make it more grepable.
> 
> 3. Extend the task comm size to 24 for CONFIG_BASE_FULL case and keep it
> as 16 for CONFIG_BASE_SMALL.

Is this justified?  How much simpler/more reliable/more maintainable/
would the code be if we were to make CONFIG_BASE_SMALL suffer with the
extra 8 bytes?

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

Re: [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL

[Kees Cook]

On Thu, Oct 21, 2021 at 08:52:22PM -0700, Andrew Morton wrote:
> On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao <laoar.shao@gmail.com> wrote:
> 
> > This patchset changes files among many subsystems. I don't know which
> > tree it should be applied to, so I just base it on Linus's tree.
> 
> I can do that ;)
> 
> > There're many truncated kthreads in the kernel, which may make trouble
> > for the user, for example, the user can't get detailed device
> > information from the task comm.
> 
> That sucked of us.
> 
> > This patchset tries to improve this problem fundamentally by extending
> > the task comm size from 16 to 24. In order to do that, we have to do
> > some cleanups first.
> 
> It's at v5 and there's no evidence of review activity?  C'mon, folks!

It's on my list! :) It's a pretty subtle area that rarely changes, so I
want to make sure I'm a full coffee to do the review. :)

> > 1. Make the copy of task comm always safe no matter what the task
> > comm size is. For example,
> > 
> >   Unsafe                 Safe
> >   strlcpy                strscpy_pad
> >   strncpy                strscpy_pad
> >   bpf_probe_read_kernel  bpf_probe_read_kernel_str
> >                          bpf_core_read_str
> >                          bpf_get_current_comm
> >                          perf_event__prepare_comm
> >                          prctl(2)
> > 
> > 2. Replace the old hard-coded 16 with a new macro TASK_COMM_LEN_16 to
> > make it more grepable.
> > 
> > 3. Extend the task comm size to 24 for CONFIG_BASE_FULL case and keep it
> > as 16 for CONFIG_BASE_SMALL.
> 
> Is this justified?  How much simpler/more reliable/more maintainable/
> would the code be if we were to make CONFIG_BASE_SMALL suffer with the
> extra 8 bytes?

Does anyone "own" CONFIG_BASE_SMALL? Gonna go with "no":

$ git ann init/Kconfig| grep 'config BASE_SMALL'
1da177e4c3f41   (Linus Torvalds 2005-04-16 15:20:36 -0700 2054)config BASE_SMALL

And it looks mostly unused:

$ git grep CONFIG_BASE_SMALL | cut -d: -f1 | sort -u | xargs -n1 git ann -f | grep 'CONFIG_BASE_SMALL'
b2af018ff26f1   (Ingo Molnar    2009-01-28 17:36:56 +0100       18)#if CONFIG_BASE_SMALL == 0
fcdba07ee390d   ( Jiri Olsa     2011-02-07 19:31:25 +0100       54)#define CON_BUF_SIZE (CONFIG_BASE_SMALL ? 256 : PAGE_SIZE)
Blaming lines: 100% (46/46), done.
1da177e4c3f41   (Linus Torvalds 2005-04-16 15:20:36 -0700       28)#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000)
1da177e4c3f41   (Linus Torvalds 2005-04-16 15:20:36 -0700       34)#define PID_MAX_LIMIT (CONFIG_BASE_SMALL ? PAGE_SIZE * 8 : \
Blaming lines: 100% (162/162), done.
f86dcc5aa8c79   (Eric Dumazet   2009-10-07 00:37:59 +0000       31)#define UDP_HTABLE_SIZE_MIN     (CONFIG_BASE_SMALL ? 128 : 256)
02c02bf12c5d8   (Matthew Wilcox 2017-11-03 23:09:45 -0400       1110)#define XA_CHUNK_SHIFT        (CONFIG_BASE_SMALL ? 4 : 6)
a52b89ebb6d44   (Davidlohr Bueso        2014-01-12 15:31:23 -0800       4249)#if CONFIG_BASE_SMALL
7b44ab978b77a   (Eric W. Biederman      2011-11-16 23:20:58 -0800       78)#define UIDHASH_BITS (CONFIG_BASE_SMALL ? 3 : 7)


-- 
Kees Cook

Re: [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL

[Yafang Shao]

On Fri, Oct 22, 2021 at 12:00 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Oct 21, 2021 at 08:52:22PM -0700, Andrew Morton wrote:
> > On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > > This patchset changes files among many subsystems. I don't know which
> > > tree it should be applied to, so I just base it on Linus's tree.
> >
> > I can do that ;)
> >
> > > There're many truncated kthreads in the kernel, which may make trouble
> > > for the user, for example, the user can't get detailed device
> > > information from the task comm.
> >
> > That sucked of us.
> >
> > > This patchset tries to improve this problem fundamentally by extending
> > > the task comm size from 16 to 24. In order to do that, we have to do
> > > some cleanups first.
> >
> > It's at v5 and there's no evidence of review activity?  C'mon, folks!
>
> It's on my list! :) It's a pretty subtle area that rarely changes, so I
> want to make sure I'm a full coffee to do the review. :)
>
> > > 1. Make the copy of task comm always safe no matter what the task
> > > comm size is. For example,
> > >
> > >   Unsafe                 Safe
> > >   strlcpy                strscpy_pad
> > >   strncpy                strscpy_pad
> > >   bpf_probe_read_kernel  bpf_probe_read_kernel_str
> > >                          bpf_core_read_str
> > >                          bpf_get_current_comm
> > >                          perf_event__prepare_comm
> > >                          prctl(2)
> > >
> > > 2. Replace the old hard-coded 16 with a new macro TASK_COMM_LEN_16 to
> > > make it more grepable.
> > >
> > > 3. Extend the task comm size to 24 for CONFIG_BASE_FULL case and keep it
> > > as 16 for CONFIG_BASE_SMALL.
> >
> > Is this justified?  How much simpler/more reliable/more maintainable/
> > would the code be if we were to make CONFIG_BASE_SMALL suffer with the
> > extra 8 bytes?
>
> Does anyone "own" CONFIG_BASE_SMALL? Gonna go with "no":
>
> $ git ann init/Kconfig| grep 'config BASE_SMALL'
> 1da177e4c3f41   (Linus Torvalds 2005-04-16 15:20:36 -0700 2054)config BASE_SMALL
>
> And it looks mostly unused:
>
> $ git grep CONFIG_BASE_SMALL | cut -d: -f1 | sort -u | xargs -n1 git ann -f | grep 'CONFIG_BASE_SMALL'
> b2af018ff26f1   (Ingo Molnar    2009-01-28 17:36:56 +0100       18)#if CONFIG_BASE_SMALL == 0
> fcdba07ee390d   ( Jiri Olsa     2011-02-07 19:31:25 +0100       54)#define CON_BUF_SIZE (CONFIG_BASE_SMALL ? 256 : PAGE_SIZE)
> Blaming lines: 100% (46/46), done.
> 1da177e4c3f41   (Linus Torvalds 2005-04-16 15:20:36 -0700       28)#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000)
> 1da177e4c3f41   (Linus Torvalds 2005-04-16 15:20:36 -0700       34)#define PID_MAX_LIMIT (CONFIG_BASE_SMALL ? PAGE_SIZE * 8 : \
> Blaming lines: 100% (162/162), done.
> f86dcc5aa8c79   (Eric Dumazet   2009-10-07 00:37:59 +0000       31)#define UDP_HTABLE_SIZE_MIN     (CONFIG_BASE_SMALL ? 128 : 256)
> 02c02bf12c5d8   (Matthew Wilcox 2017-11-03 23:09:45 -0400       1110)#define XA_CHUNK_SHIFT        (CONFIG_BASE_SMALL ? 4 : 6)
> a52b89ebb6d44   (Davidlohr Bueso        2014-01-12 15:31:23 -0800       4249)#if CONFIG_BASE_SMALL
> 7b44ab978b77a   (Eric W. Biederman      2011-11-16 23:20:58 -0800       78)#define UIDHASH_BITS (CONFIG_BASE_SMALL ? 3 : 7)
>
>
> --

Right. CONFIG_BASE_SMALL is seldomly used in the kernel.
As you have already removed 64 bytes from task_struct, I think we can
extend the 8 bytes for CONFIG_BASE_SMALL as well.

-- 
Thanks
Yafang