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
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 <redacted>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <redacted>
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(-)
@@ -1209,7 +1209,8 @@ static int unshare_sighand(struct task_struct *me)char*__get_task_comm(char*buf,size_tbuf_size,structtask_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);returnbuf;}
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 <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
include/linux/sched.h | 2 ++
1 file changed, 2 insertions(+)
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 <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Vladimir Zapolskiy <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Kees Cook <redacted>
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(-)
@@ -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 */
@@ -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+/**Userspacesendsthisenumtoregisterwiththekernelthatitislistening*foreventsontheconnector.
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 <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <redacted>
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(-)
@@ -195,8 +195,8 @@ struct qib_ctxtdata {/* pid of process using this ctxt */pid_tpid;pid_tsubpid[QLOGIC_IB_MAX_SUBCTXT];-/* same size as task_struct .comm[], command that opened context */-charcomm[16];+/* task_struct .comm[], command that opened context */+charcomm[TASK_COMM_LEN_16];/* pkeys set by this use of this ctxt */u16pkeys[4];/* so file ops can get at unit */
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':
quoted
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 <redacted>
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <redacted>
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(-)
@@ -65,8 +65,8 @@ struct elf_prpsinfo__kernel_gid_tpr_gid;pid_tpr_pid,pr_ppid,pr_pgrp,pr_sid;/* Lots missing */-charpr_fname[16];/* filename of executable */-charpr_psargs[ELF_PRARGSZ];/* initial part of arg list */+charpr_fname[TASK_COMM_LEN_16];/* filename of executable */+charpr_psargs[ELF_PRARGSZ];/* initial part of arg list */};staticinlinevoidelf_core_copy_regs(elf_gregset_t*elfregs,structpt_regs*regs)
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 <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <redacted>
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(-)
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 <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <redacted>
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(+)
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 <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <redacted>
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(-)
@@ -28,7 +29,7 @@ static void stars(char *str, long val, long max, int width)}structtask{-charcomm[16];+charcomm[TASK_COMM_LEN_16];__u64pid_tgid;__u64uid_gid;};
On Wed, Oct 20, 2021 at 8:45 PM Yafang Shao [off-list ref] wrote:
quoted hunk
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 <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
include/linux/sched.h | 2 ++
1 file changed, 2 insertions(+)
@@ -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.
From: Andrew Morton <akpm@linux-foundation.org> Date: 2021-10-22 03:52:29
On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao [off-list ref] 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.
On Thu, Oct 21, 2021 at 08:52:22PM -0700, Andrew Morton wrote:
On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao [off-list ref] wrote:
quoted
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 ;)
quoted
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.
quoted
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. :)
quoted
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?
On Fri, Oct 22, 2021 at 12:00 PM Kees Cook [off-list ref] wrote:
On Thu, Oct 21, 2021 at 08:52:22PM -0700, Andrew Morton wrote:
quoted
On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao [off-list ref] wrote:
quoted
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 ;)
quoted
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.
quoted
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. :)
quoted
quoted
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?
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
On Fri, Oct 22, 2021 at 5:55 AM Andrii Nakryiko
[off-list ref] wrote:
On Wed, Oct 20, 2021 at 8:45 PM Yafang Shao [off-list ref] wrote:
quoted
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 <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <redacted>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
include/linux/sched.h | 2 ++
1 file changed, 2 insertions(+)
@@ -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.