There're many truncated kthreads in the kernel, which may make trouble
for the user, for example, the user can't get detailed device
information from the task comm.
This patchset tries to improve this problem fundamentally by extending
the task comm size from 16 to 24, which is a very simple way.
In order to do that, we have to do some cleanups first.
1. Make the copy of task comm always safe no matter what the task
comm size is. For example,
Unsafe Safe
strlcpy strscpy_pad
strncpy strscpy_pad
bpf_probe_read_kernel bpf_probe_read_kernel_str
bpf_core_read_str
bpf_get_current_comm
perf_event__prepare_comm
prctl(2)
After this step, the comm size change won't make any trouble to the
kernel or the in-tree tools for example perf, BPF programs.
2. Cleanup some old hard-coded 16
Actually we don't need to convert all of them to TASK_COMM_LEN or
TASK_COMM_LEN_16, what we really care about is if the convert can
make the code more reasonable or easier to understand. For
example, some in-tree tools read the comm from sched:sched_switch
tracepoint, as it is derived from the kernel, we'd better make them
consistent with the kernel.
3. Extend the task comm size from 16 to 24
task_struct is growing rather regularly by 8 bytes. This size change
should be acceptable. We used to think about extending the size for
CONFIG_BASE_FULL only, but that would be a burden for maintenance
and introduce code complexity.
4. Print a warning if the kthread comm is still truncated.
5. What will happen to the out-of-tree tools after this change?
If the tool get task comm through kernel API, for example prctl(2),
bpf_get_current_comm() and etc, then it doesn't matter how large the
user buffer is, because it will always get a string with a nul
terminator. While if it gets the task comm through direct string copy,
the user tool must make sure the copied string has a nul terminator
itself. As TASK_COMM_LEN is not exposed to userspace, there's no
reason that it must require a fixed-size task comm.
Changes since v6:
Various suggestion from Kees:
- replace strscpy_pad() with the helper get_task_comm()
- fix typo
- replace BUILD_BUG_ON() with __must_be_array()
- don't change the size of pr_fname
- merge two samples/bpf/ patches to one patch
- keep TASK_COMM_LEN_16 per
Changes since v5:
- extend the comm size for both CONFIG_BASE_{FULL, SMALL} that could
make the code more simple and easier to maintain.
- avoid changing too much hard-coded 16 in BPF programs per Andrii.
Changes since v4:
- introduce TASK_COMM_LEN_16 and TASK_COMM_LEN_24 per Steven
- replace hard-coded 16 with TASK_COMM_LEN_16 per Kees
- use strscpy_pad() instead of strlcpy()/strncpy() per Kees
- make perf test adopt to task comm size change per Arnaldo and Mathieu
- fix warning reported by kernel test robot
Changes since v3:
- fixes -Wstringop-truncation warning reported by kernel test robot
Changes since v2:
- avoid change UAPI code per Kees
- remove the description of out of tree code from commit log per Peter
Changes since v1:
- extend task comm to 24bytes, per Petr
- improve the warning per Petr
- make the checkpatch warning a separate patch
Yafang Shao (11):
fs/exec: make __set_task_comm always set a nul terminated string
fs/exec: make __get_task_comm always get a nul terminated string
sched.h: use __must_be_array instead of BUILD_BUG_ON in get_task_comm
drivers/infiniband: make setup_ctxt always get a nul terminated task
comm
fs/binfmt_elf: make prpsinfo always get a nul terminated task comm
samples/bpf/test_overhead_kprobe_kern: make it adopt to task comm size
change
tools/bpf/bpftool/skeleton: make it adopt to task comm size change
tools/perf/test: make perf test adopt to task comm size change
tools/testing/selftests/bpf: make it adopt to task comm size change
sched.h: extend task comm from 16 to 24
kernel/kthread: show a warning if kthread's comm is truncated
drivers/infiniband/hw/qib/qib.h | 2 +-
drivers/infiniband/hw/qib/qib_file_ops.c | 2 +-
fs/binfmt_elf.c | 2 +-
fs/exec.c | 5 ++--
include/linux/sched.h | 16 +++++++-----
kernel/kthread.c | 7 ++++-
samples/bpf/offwaketime_kern.c | 4 +--
samples/bpf/test_overhead_kprobe_kern.c | 11 ++++----
samples/bpf/test_overhead_tp_kern.c | 5 ++--
tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 4 +--
tools/include/linux/sched.h | 11 ++++++++
tools/perf/tests/evsel-tp-sched.c | 26 ++++++++++++++-----
.../selftests/bpf/progs/test_stacktrace_map.c | 6 ++---
.../selftests/bpf/progs/test_tracepoint.c | 6 ++---
14 files changed, 72 insertions(+), 35 deletions(-)
create mode 100644 tools/include/linux/sched.h
--
2.17.1
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>
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: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
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(-)
Now that __get_task_comm() will truncate and pad, it's safe to use on both
too-small and too-big arrays. So it is not needed to check array length
any more. For the original goal of making sure get_task_comm() is being
used on a char array, we can use __must_be_array().
Below is the verification when I changed the dest buffer of
get_task_comm() in a driver code,
CC [M] drivers/infiniband/hw/qib/qib_file_ops.o
In file included from ./include/linux/bits.h:22:0,
from ./include/linux/ioport.h:13,
from ./include/linux/pci.h:31,
from drivers/infiniband/hw/qib/qib_file_ops.c:35:
drivers/infiniband/hw/qib/qib_file_ops.c: In function ‘setup_ctxt’:
./include/linux/build_bug.h:16:51: error: negative width in bit-field ‘<anonymous>’
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
./include/linux/compiler.h:258:28: note: in expansion of macro ‘BUILD_BUG_ON_ZERO’
#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
^~~~~~~~~~~~~~~~~
./include/linux/sched.h:1941:23: note: in expansion of macro ‘__must_be_array’
__get_task_comm(buf, __must_be_array(buf) + sizeof(buf), tsk)
^~~~~~~~~~~~~~~
drivers/infiniband/hw/qib/qib_file_ops.c:1325:2: note: in expansion of macro ‘get_task_comm’
get_task_comm(test, current);
It hit this warnig as expected.
Suggested-by: Kees Cook <redacted>
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---
include/linux/sched.h | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
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: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
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(-)
@@ -196,7 +196,7 @@ struct qib_ctxtdata {pid_tpid;pid_tsubpid[QLOGIC_IB_MAX_SUBCTXT];/* same size as task_struct .comm[], command that opened context */-charcomm[16];+charcomm[TASK_COMM_LEN];/* 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));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Replacing the open-coded strncpy() with the helper get_task_comm() can
avoid this warning.
As the struct prpsinfo is externally parsed, for example by the crash
utility, we can't change the size of pr_fname. So I just leave it as-is.
I also verfied if it still work well when I extend the comm size to 24.
struct elf_prpsinfo is used to dump the task information in userspace
coredump or kernel vmcore. Below is the verfication of vmcore,
crash> ps
PID PPID CPU TASK ST %MEM VSZ RSS COMM
0 0 0 ffffffff9d21a940 RU 0.0 0 0 [swapper/0]
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>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
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(-)
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: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
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(-)
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>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
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(-)
@@ -41,11 +41,11 @@ struct {/* taken from /sys/kernel/debug/tracing/events/sched/sched_switch/format */structsched_switch_args{unsignedlonglongpad;-charprev_comm[16];+charprev_comm[TASK_COMM_LEN];intprev_pid;intprev_prio;longlongprev_state;-charnext_comm[16];+charnext_comm[TASK_COMM_LEN];intnext_pid;intnext_prio;};
When I was implementing a new per-cpu kthread cfs_migration, I found the
comm of it "cfs_migration/%u" is truncated due to the limitation of
TASK_COMM_LEN. For example, the comm of the percpu thread on CPU10~19 are
all with the same name "cfs_migration/1", which will confuse the user. This
issue is not critical, because we can get the corresponding CPU from the
task's Cpus_allowed. But for kthreads correspoinding to other hardware
devices, it is not easy to get the detailed device info from task comm,
for example,
jbd2/nvme0n1p2-
xfs-reclaim/sdf
We can also shorten the name to work around this problem, but I find
there are so many truncated kthreads:
rcu_tasks_kthre
rcu_tasks_rude_
rcu_tasks_trace
poll_mpt3sas0_s
ext4-rsv-conver
xfs-reclaim/sd{a, b, c, ...}
xfs-blockgc/sd{a, b, c, ...}
xfs-inodegc/sd{a, b, c, ...}
audit_send_repl
ecryptfs-kthrea
vfio-irqfd-clea
jbd2/nvme0n1p2-
...
We should improve this problem fundamentally by extending comm size to
24 bytes. task_struct is growing rather regularly by 8 bytes.
After this change, the truncated kthreads listed above will be
displayed as:
rcu_tasks_kthread
rcu_tasks_rude_kthread
rcu_tasks_trace_kthread
poll_mpt3sas0_statu
ext4-rsv-conversion
xfs-reclaim/sdf1
xfs-blockgc/sdf1
xfs-inodegc/sdf1
audit_send_reply
ecryptfs-kthread
vfio-irqfd-cleanup
jbd2/nvme0n1p2-8
As we have converted all the unsafe copy of task comm to the safe one,
this change won't make any trouble to the kernel or the in-tree tools.
The safe one and unsafe one of comm copy as follows,
Unsafe Safe
strlcpy strscpy_pad
strncpy strscpy_pad
bpf_probe_read_kernel bpf_probe_read_kernel_str
bpf_core_read_str
bpf_get_current_comm
perf_event__prepare_comm
prctl(2)
Regarding the possible risk it may take to the out-of-tree user tools, if
the user tools get the task comm through kernel API like prctl(2),
bpf_get_current_comm() and etc, the tools still work well after this
change. While If the user tools get the task comm through direct string
copy, it must make sure the copied string should be with a nul terminator.
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---
include/linux/sched.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
kernel test robot reported a perf-test failure after I extended task comm
size from 16 to 24. The failure as follows,
2021-10-13 18:00:46 sudo /usr/src/perf_selftests-x86_64-rhel-8.3-317419b91ef4eff4e2f046088201e4dc4065caa0/tools/perf/perf test 15
15: Parse sched tracepoints fields : FAILED!
The reason is perf-test requires a fixed-size task comm. If we extend
task comm size to 24, it will not equil with the required size 16 in perf
test.
After some analyzation, I found perf itself can adopt to the size
change, for example, below is the output of perf-sched after I extend
comm size to 24 -
task 614 ( kthreadd: 84), nr_events: 1
task 615 ( systemd: 843), nr_events: 1
task 616 ( networkd-dispat: 1026), nr_events: 1
task 617 ( systemd: 846), nr_events: 1
$ cat /proc/843/comm
networkd-dispatcher
The task comm can be displayed correctly as expected.
Replace old hard-coded 16 with the new one can fix the warning, but we'd
better make the test accept both old and new sizes, then it can be
backward compatibility.
After this patch, the perf-test succeeds no matter task comm is 16 or
24 -
15: Parse sched tracepoints fields : Ok
This patch is a preparation for the followup patch.
Reported-by: kernel test robot <redacted>
Suggested-by: Arnaldo Carvalho de Melo <redacted>
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---
tools/include/linux/sched.h | 11 +++++++++++
tools/perf/tests/evsel-tp-sched.c | 26 ++++++++++++++++++++------
2 files changed, 31 insertions(+), 6 deletions(-)
create mode 100644 tools/include/linux/sched.h
@@ -23,15 +25,24 @@ static int evsel__test_field(struct evsel *evsel, const char *name, int size, boret=-1;}-if(field->size!=size){-pr_debug("%s: \"%s\" size (%d) should be %d!\n",+if(field->size!=size&&field->size!=alternate_size){+pr_debug("%s: \"%s\" size (%d) should be %d",evsel->name,name,field->size,size);+if(alternate_size>0)+pr_debug(" or %d",alternate_size);+pr_debug("!\n");ret=-1;}returnret;}+staticintevsel__test_field(structevsel*evsel,constchar*name,+intsize,boolshould_be_signed)+{+returnevsel__test_field_alt(evsel,name,size,-1,should_be_signed);+}+inttest__perf_evsel__tp_sched_test(structtest*test__maybe_unused,intsubtest__maybe_unused){structevsel*evsel=evsel__newtp("sched","sched_switch");
@@ -42,7 +53,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtesreturn-1;}-if(evsel__test_field(evsel,"prev_comm",16,false))+if(evsel__test_field_alt(evsel,"prev_comm",TASK_COMM_LEN_16,+TASK_COMM_LEN,false))ret=-1;if(evsel__test_field(evsel,"prev_pid",4,true))
@@ -54,7 +66,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtesif(evsel__test_field(evsel,"prev_state",sizeof(long),true))ret=-1;-if(evsel__test_field(evsel,"next_comm",16,false))+if(evsel__test_field_alt(evsel,"next_comm",TASK_COMM_LEN_16,+TASK_COMM_LEN,false))ret=-1;if(evsel__test_field(evsel,"next_pid",4,true))
@@ -72,7 +85,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtesreturn-1;}-if(evsel__test_field(evsel,"comm",16,false))+if(evsel__test_field_alt(evsel,"comm",TASK_COMM_LEN_16,+TASK_COMM_LEN,false))ret=-1;if(evsel__test_field(evsel,"pid",4,true))
Show a warning if task comm is truncated. Below is the result
of my test case:
truncated kthread comm:I-am-a-kthread-with-a-l (pid:178) by 8 characters
As we have extended task comm to 24, all the existing in-tree
kthreads/workqueues are not truncated anymore. So this warning will only
be printed for the newly introduced one if it has a long name.
Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Yafang Shao <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: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---
kernel/kthread.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
From: Matthew Wilcox <willy@infradead.org> Date: 2021-11-01 12:50:51
On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
There're many truncated kthreads in the kernel, which may make trouble
for the user, for example, the user can't get detailed device
information from the task comm.
This patchset tries to improve this problem fundamentally by extending
the task comm size from 16 to 24, which is a very simple way.
It can't be that simple if we're on v7 and at 11 patches!
It would be helpful if you included links to earlier postings. I can
only find v5 and v6 in my inbox, so I fear I'm going to re-ask some
questions which were already answered.
Why can't we shorten the names of these kthreads? You didn't
give any examples, so I can't suggest any possibilities.
On Mon, Nov 1, 2021 at 8:46 PM Matthew Wilcox [off-list ref] wrote:
On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
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.
This patchset tries to improve this problem fundamentally by extending
the task comm size from 16 to 24, which is a very simple way.
It can't be that simple if we're on v7 and at 11 patches!
Most of the changes are because of hard-coded 16 that can't be easily grepped.
In these 11 patches, patch #1, #2, #4, #5, #6, #7 and #9 are cleanups,
which can be a different patchset.
The core changes of these patchset are patch #3, #8 and #10.
#11 can also be a seperate patch.
It would be helpful if you included links to earlier postings. I can
only find v5 and v6 in my inbox, so I fear I'm going to re-ask some
questions which were already answered.
Why can't we shorten the names of these kthreads? You didn't
give any examples, so I can't suggest any possibilities.
Take 'jbd2/nvme0n1p2-' for example, that's a nice name, which gives a
good description via the name.
And I don't think it is a good idea to shorten its name.
--
Thanks
Yafang
From: Petr Mladek <pmladek@suse.com> Date: 2021-11-01 14:07:44
On Mon 2021-11-01 06:04:08, Yafang Shao wrote:
There're many truncated kthreads in the kernel, which may make trouble
for the user, for example, the user can't get detailed device
information from the task comm.
This patchset tries to improve this problem fundamentally by extending
the task comm size from 16 to 24, which is a very simple way.
In order to do that, we have to do some cleanups first.
1. Make the copy of task comm always safe no matter what the task
comm size is. For example,
Unsafe Safe
strlcpy strscpy_pad
strncpy strscpy_pad
bpf_probe_read_kernel bpf_probe_read_kernel_str
bpf_core_read_str
bpf_get_current_comm
perf_event__prepare_comm
prctl(2)
After this step, the comm size change won't make any trouble to the
kernel or the in-tree tools for example perf, BPF programs.
2. Cleanup some old hard-coded 16
Actually we don't need to convert all of them to TASK_COMM_LEN or
TASK_COMM_LEN_16, what we really care about is if the convert can
make the code more reasonable or easier to understand. For
example, some in-tree tools read the comm from sched:sched_switch
tracepoint, as it is derived from the kernel, we'd better make them
consistent with the kernel.
The above changes make sense even if we do not extend comm[] array in
task_struct.
3. Extend the task comm size from 16 to 24
task_struct is growing rather regularly by 8 bytes. This size change
should be acceptable. We used to think about extending the size for
CONFIG_BASE_FULL only, but that would be a burden for maintenance
and introduce code complexity.
4. Print a warning if the kthread comm is still truncated.
5. What will happen to the out-of-tree tools after this change?
If the tool get task comm through kernel API, for example prctl(2),
bpf_get_current_comm() and etc, then it doesn't matter how large the
user buffer is, because it will always get a string with a nul
terminator. While if it gets the task comm through direct string copy,
the user tool must make sure the copied string has a nul terminator
itself. As TASK_COMM_LEN is not exposed to userspace, there's no
reason that it must require a fixed-size task comm.
The amount of code that has to be updated is really high. I am pretty
sure that there are more potential buffer overflows left.
You did not commented on the concerns in the thread
https://lore.kernel.org/all/CAADnVQKm0Ljj-w5PbkAu1ugLFnZRRPt-Vk-J7AhXxDD5xVompA@mail.gmail.com/
Several people suggested to use a more conservative approach. I mean
to keep comm[16] as is and add a new pointer to the full name. The buffer
for the long name might be dynamically allocated only when needed.
The pointer might be either in task_struct or struct kthread. It might
be used the same way as the full name stored by workqueue kthreads.
The advantage of the separate pointer:
+ would work for names longer than 32
+ will not open security holes in code
Best Regards,
Petr
On Mon, Nov 1, 2021 at 10:07 PM Petr Mladek [off-list ref] wrote:
On Mon 2021-11-01 06:04:08, Yafang Shao wrote:
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.
This patchset tries to improve this problem fundamentally by extending
the task comm size from 16 to 24, which is a very simple way.
In order to do that, we have to do some cleanups first.
1. Make the copy of task comm always safe no matter what the task
comm size is. For example,
Unsafe Safe
strlcpy strscpy_pad
strncpy strscpy_pad
bpf_probe_read_kernel bpf_probe_read_kernel_str
bpf_core_read_str
bpf_get_current_comm
perf_event__prepare_comm
prctl(2)
After this step, the comm size change won't make any trouble to the
kernel or the in-tree tools for example perf, BPF programs.
2. Cleanup some old hard-coded 16
Actually we don't need to convert all of them to TASK_COMM_LEN or
TASK_COMM_LEN_16, what we really care about is if the convert can
make the code more reasonable or easier to understand. For
example, some in-tree tools read the comm from sched:sched_switch
tracepoint, as it is derived from the kernel, we'd better make them
consistent with the kernel.
The above changes make sense even if we do not extend comm[] array in
task_struct.
quoted
3. Extend the task comm size from 16 to 24
task_struct is growing rather regularly by 8 bytes. This size change
should be acceptable. We used to think about extending the size for
CONFIG_BASE_FULL only, but that would be a burden for maintenance
and introduce code complexity.
4. Print a warning if the kthread comm is still truncated.
5. What will happen to the out-of-tree tools after this change?
If the tool get task comm through kernel API, for example prctl(2),
bpf_get_current_comm() and etc, then it doesn't matter how large the
user buffer is, because it will always get a string with a nul
terminator. While if it gets the task comm through direct string copy,
the user tool must make sure the copied string has a nul terminator
itself. As TASK_COMM_LEN is not exposed to userspace, there's no
reason that it must require a fixed-size task comm.
From: Petr Mladek <pmladek@suse.com> Date: 2021-11-01 16:02:19
On Mon 2021-11-01 22:34:30, Yafang Shao wrote:
On Mon, Nov 1, 2021 at 10:07 PM Petr Mladek [off-list ref] wrote:
quoted
On Mon 2021-11-01 06:04:08, Yafang Shao wrote:
quoted
4. Print a warning if the kthread comm is still truncated.
5. What will happen to the out-of-tree tools after this change?
If the tool get task comm through kernel API, for example prctl(2),
bpf_get_current_comm() and etc, then it doesn't matter how large the
user buffer is, because it will always get a string with a nul
terminator. While if it gets the task comm through direct string copy,
the user tool must make sure the copied string has a nul terminator
itself. As TASK_COMM_LEN is not exposed to userspace, there's no
reason that it must require a fixed-size task comm.
Honestly, I am a bit confused by Kees' answer. IMHO, he agreed that
switching task->comm[16] into a pointer was not worth it.
But I am not sure what he meant by "Agreed -- this is a small change
for what is already an "uncommon" corner case."
quoted
Several people suggested to use a more conservative approach.
Alexei used dentry->d_iname as an exaxmple. struct dentry uses
d_iname[DNAME_INLINE_LEN] for short names. And dynamically
allocated d_name for long names, see *__d_alloc() implementation.
quoted
I mean
to keep comm[16] as is and add a new pointer to the full name. The buffer
for the long name might be dynamically allocated only when needed.
That would add a new allocation in the fork() for the threads with a long name.
I'm not sure if it is worth it.
The allocation will be done only when needed. IMHO, the performance is
important only for userspace processes. I am not aware of any kernel
subsystem that would heavily create and destroy kthreads.
quoted
The pointer might be either in task_struct or struct kthread. It might
be used the same way as the full name stored by workqueue kthreads.
If we decide to do it like that, I think we'd better add it in
task_struct, then it will work for all tasks.
Is it really needed for userspace processes? For example, ps shows
the information from /proc/*/cmdline instead.
quoted
The advantage of the separate pointer:
+ would work for names longer than 32
+ will not open security holes in code
Yes, those are the advantages. And the disadvantage of it is:
- new allocation in fork()
It should not be a problem if we do it only when necessary and only
for kthreads.
Best Regards,
Petr
Steven was against switching task->comm[16] into a dynamically
allocated pointer. But he was not against storing longer names
separately.
Just to be clear. I was recommending that the comm[16] would still behave
like it does today. Where it is truncated. But if the name is longer, it
could be stored in a separate location if the caller wanted to know the
full name.
-- Steve
On Sun, Oct 31, 2021 at 11:04 PM Yafang Shao [off-list ref] 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>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---
On Sun, Oct 31, 2021 at 11:04 PM 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.
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---
On Tue, Nov 2, 2021 at 12:02 AM Petr Mladek [off-list ref] wrote:
On Mon 2021-11-01 22:34:30, Yafang Shao wrote:
quoted
On Mon, Nov 1, 2021 at 10:07 PM Petr Mladek [off-list ref] wrote:
quoted
On Mon 2021-11-01 06:04:08, Yafang Shao wrote:
quoted
4. Print a warning if the kthread comm is still truncated.
5. What will happen to the out-of-tree tools after this change?
If the tool get task comm through kernel API, for example prctl(2),
bpf_get_current_comm() and etc, then it doesn't matter how large the
user buffer is, because it will always get a string with a nul
terminator. While if it gets the task comm through direct string copy,
the user tool must make sure the copied string has a nul terminator
itself. As TASK_COMM_LEN is not exposed to userspace, there's no
reason that it must require a fixed-size task comm.
Honestly, I am a bit confused by Kees' answer. IMHO, he agreed that
switching task->comm[16] into a pointer was not worth it.
But I am not sure what he meant by "Agreed -- this is a small change
for what is already an "uncommon" corner case."
quoted
quoted
Several people suggested to use a more conservative approach.
Alexei used dentry->d_iname as an exaxmple. struct dentry uses
d_iname[DNAME_INLINE_LEN] for short names. And dynamically
allocated d_name for long names, see *__d_alloc() implementation.
I mean
to keep comm[16] as is and add a new pointer to the full name. The buffer
for the long name might be dynamically allocated only when needed.
That would add a new allocation in the fork() for the threads with a long name.
I'm not sure if it is worth it.
The allocation will be done only when needed. IMHO, the performance is
important only for userspace processes. I am not aware of any kernel
subsystem that would heavily create and destroy kthreads.
XFS may create many kthreads with longer names, especially if there're
many partitions in the disk.
For example,
xfs-reclaim/sd{a, b, c, ...}
xfs-blockgc/sd{a, b, c, ...}
xfs-inodegc/sd{a, b, c, ...}
They are supposed to be created at boot time, and shouldn't be heavily
created and destroyed.
quoted
quoted
The pointer might be either in task_struct or struct kthread. It might
be used the same way as the full name stored by workqueue kthreads.
If we decide to do it like that, I think we'd better add it in
task_struct, then it will work for all tasks.
Is it really needed for userspace processes? For example, ps shows
the information from /proc/*/cmdline instead.
Right. The userspace processes can be obtained from /proc/*/cmdline.
quoted
quoted
The advantage of the separate pointer:
+ would work for names longer than 32
+ will not open security holes in code
Yes, those are the advantages. And the disadvantage of it is:
- new allocation in fork()
It should not be a problem if we do it only when necessary and only
for kthreads.
So if no one against, I will do it in two steps,
1. Send the task comm cleanups in a separate patchset named "task comm cleanups"
This patchset includes patch #1, #2, #4, #5, #6, #7 and #9.
Cleaning them up can make it less error prone, and it will be
helpful if we want to extend task comm in the future :)
2. Keep the current comm[16] as-is and introduce a separate pointer
to store kthread's long name
Now we only care about kthread, so we can put the pointer into a
kthread specific struct.
For example in the struct kthread, or in kthread->data (which may
conflict with workqueue).
And then dynamically allocate a longer name if it is truncated,
for example,
__kthread_create_on_node
len = vsnprintf(name, sizeof(name), namefmt, args);
if (len >= TASK_COMM_LEN) {
/* create a longer name */
}
And then we modify proc_task_name(), so the user can get
kthread's longer name via /proc/[pid]/comm.
And then free the allocated memory when the kthread is destroyed.
--
Thanks
Yafang
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-11-02 01:18:55
On Tue, 2 Nov 2021 09:09:50 +0800
Yafang Shao [off-list ref] wrote:
So if no one against, I will do it in two steps,
1. Send the task comm cleanups in a separate patchset named "task comm cleanups"
This patchset includes patch #1, #2, #4, #5, #6, #7 and #9.
Cleaning them up can make it less error prone, and it will be
helpful if we want to extend task comm in the future :)
Agreed.
2. Keep the current comm[16] as-is and introduce a separate pointer
to store kthread's long name
I'm OK with this. Hopefully more would chime in too.
Now we only care about kthread, so we can put the pointer into a
kthread specific struct.
For example in the struct kthread, or in kthread->data (which may
conflict with workqueue).
No, add a new field to the structure. "full_name" or something like that.
I'm guessing it should be NULL if the name fits in TASK_COMM_LEN and
allocated if the name had to be truncated.
Do not overload data with this. That will just make things confusing.
There's not that many kthreads, where an addition of an 8 byte pointer is
going to cause issues.
And then dynamically allocate a longer name if it is truncated,
for example,
__kthread_create_on_node
len = vsnprintf(name, sizeof(name), namefmt, args);
if (len >= TASK_COMM_LEN) {
/* create a longer name */
And make sure you have it fail the kthread allocation if it fails to
allocate.
}
And then we modify proc_task_name(), so the user can get
kthread's longer name via /proc/[pid]/comm.
Agreed.
And then free the allocated memory when the kthread is destroyed.
On Tue, Nov 2, 2021 at 9:18 AM Steven Rostedt [off-list ref] wrote:
On Tue, 2 Nov 2021 09:09:50 +0800
Yafang Shao [off-list ref] wrote:
quoted
So if no one against, I will do it in two steps,
1. Send the task comm cleanups in a separate patchset named "task comm cleanups"
This patchset includes patch #1, #2, #4, #5, #6, #7 and #9.
Cleaning them up can make it less error prone, and it will be
helpful if we want to extend task comm in the future :)
Agreed.
quoted
2. Keep the current comm[16] as-is and introduce a separate pointer
to store kthread's long name
I'm OK with this. Hopefully more would chime in too.
quoted
Now we only care about kthread, so we can put the pointer into a
kthread specific struct.
For example in the struct kthread, or in kthread->data (which may
conflict with workqueue).
No, add a new field to the structure. "full_name" or something like that.
I'm guessing it should be NULL if the name fits in TASK_COMM_LEN and
allocated if the name had to be truncated.
Do not overload data with this. That will just make things confusing.
There's not that many kthreads, where an addition of an 8 byte pointer is
going to cause issues.
Sure, I will add a new field named "full_name", which only be
allocated if the kthread's comm is truncated.
quoted
And then dynamically allocate a longer name if it is truncated,
for example,
__kthread_create_on_node
len = vsnprintf(name, sizeof(name), namefmt, args);
if (len >= TASK_COMM_LEN) {
/* create a longer name */
And make sure you have it fail the kthread allocation if it fails to
allocate.
quoted
}
And then we modify proc_task_name(), so the user can get
kthread's longer name via /proc/[pid]/comm.
Agreed.
quoted
And then free the allocated memory when the kthread is destroyed.
From: Petr Mladek <pmladek@suse.com> Date: 2021-11-02 07:56:24
On Tue 2021-11-02 09:26:35, Yafang Shao wrote:
On Tue, Nov 2, 2021 at 9:18 AM Steven Rostedt [off-list ref] wrote:
quoted
On Tue, 2 Nov 2021 09:09:50 +0800
Yafang Shao [off-list ref] wrote:
quoted
Now we only care about kthread, so we can put the pointer into a
kthread specific struct.
For example in the struct kthread, or in kthread->data (which may
conflict with workqueue).
No, add a new field to the structure. "full_name" or something like that.
I'm guessing it should be NULL if the name fits in TASK_COMM_LEN and
allocated if the name had to be truncated.
Do not overload data with this. That will just make things confusing.
There's not that many kthreads, where an addition of an 8 byte pointer is
going to cause issues.
Sure, I will add a new field named "full_name", which only be
allocated if the kthread's comm is truncated.
The plan looks good to me.
One more thing. It should obsolete the workqueue-specific solution.
It would be great to clean up the workqueue code as the next step.
Best Regards,
Petr
On Tue, Nov 2, 2021 at 3:56 PM Petr Mladek [off-list ref] wrote:
On Tue 2021-11-02 09:26:35, Yafang Shao wrote:
quoted
On Tue, Nov 2, 2021 at 9:18 AM Steven Rostedt [off-list ref] wrote:
quoted
On Tue, 2 Nov 2021 09:09:50 +0800
Yafang Shao [off-list ref] wrote:
quoted
Now we only care about kthread, so we can put the pointer into a
kthread specific struct.
For example in the struct kthread, or in kthread->data (which may
conflict with workqueue).
No, add a new field to the structure. "full_name" or something like that.
I'm guessing it should be NULL if the name fits in TASK_COMM_LEN and
allocated if the name had to be truncated.
Do not overload data with this. That will just make things confusing.
There's not that many kthreads, where an addition of an 8 byte pointer is
going to cause issues.
Sure, I will add a new field named "full_name", which only be
allocated if the kthread's comm is truncated.
The plan looks good to me.
One more thing. It should obsolete the workqueue-specific solution.
It would be great to clean up the workqueue code as the next step.
Agreed. The worker comm can be replaced by the new kthread full_name.
I will do it in the next step.
--
Thanks
Yafang
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: 2021-11-04 01:37:23
On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
There're many truncated kthreads in the kernel, which may make trouble
for the user, for example, the user can't get detailed device
information from the task comm.
This patchset tries to improve this problem fundamentally by extending
the task comm size from 16 to 24, which is a very simple way.
[...]
Hi,
I've tried something like this a few years back. My attempt got mostly
lost in the mailing lists, but I'm still carrying the patches in my
tree [1]. My target was userspace thread names, and it turned out more
involved than I had time for.
[1] https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a
and its parents
Best Regards
Michał Mirosław
On Thu, Nov 4, 2021 at 9:37 AM Michał Mirosław [off-list ref] wrote:
On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
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.
This patchset tries to improve this problem fundamentally by extending
the task comm size from 16 to 24, which is a very simple way.
[...]
Hi,
I've tried something like this a few years back. My attempt got mostly
lost in the mailing lists, but I'm still carrying the patches in my
tree [1]. My target was userspace thread names, and it turned out more
involved than I had time for.
[1] https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a
and its parents
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: 2021-11-05 23:57:23
On Fri, Nov 05, 2021 at 02:34:58PM +0800, Yafang Shao wrote:
On Thu, Nov 4, 2021 at 9:37 AM Michał Mirosław [off-list ref] wrote:
quoted
On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
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.
This patchset tries to improve this problem fundamentally by extending
the task comm size from 16 to 24, which is a very simple way.
[...]
Hi,
I've tried something like this a few years back. My attempt got mostly
lost in the mailing lists, but I'm still carrying the patches in my
tree [1]. My target was userspace thread names, and it turned out more
involved than I had time for.
[1] https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a
and its parents
This was to enable longer thread names (as set by pthread_setname_np()).
Currently its 16 bytes, and that's too short for e.g. Chrome's or Firefox'es
threads. I believe that FreeBSD has 32-byte limit and so I expect that
major portable code is already prepared for bigger thread names.
Currently writes to /proc/PID/comm are silently truncated. This patch
makes the write() call return the actual number of bytes actually written
and on subsequent calls return -ENOSPC. glibc checks the length in
pthread_setname_np() before write(), so the change is not currently
relevant for it. I don't know/remember what other runtimes do, though.
This one I'm not really fond of because it abuses lseek() in that it
doesn't move the write pointer. But in case of /proc files this normally
would return EINVAL anyway.
Best Regards
Michał Mirosław
On Sat, Nov 6, 2021 at 7:57 AM Michał Mirosław [off-list ref] wrote:
On Fri, Nov 05, 2021 at 02:34:58PM +0800, Yafang Shao wrote:
quoted
On Thu, Nov 4, 2021 at 9:37 AM Michał Mirosław [off-list ref] wrote:
quoted
On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
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.
This patchset tries to improve this problem fundamentally by extending
the task comm size from 16 to 24, which is a very simple way.
[...]
Hi,
I've tried something like this a few years back. My attempt got mostly
lost in the mailing lists, but I'm still carrying the patches in my
tree [1]. My target was userspace thread names, and it turned out more
involved than I had time for.
[1] https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a
and its parents
This was to enable longer thread names (as set by pthread_setname_np()).
Currently its 16 bytes, and that's too short for e.g. Chrome's or Firefox'es
threads. I believe that FreeBSD has 32-byte limit and so I expect that
major portable code is already prepared for bigger thread names.
The comm len in FreeBSD is (19 + 1) bytes[1], but that is still larger
than Linux :)
The task comm is short for many applications, that is why cmdline is
introduced per my understanding, but pthread_{set, get}name_np() is
reading/writing the comm or via prctl(2) rather than reading/writing
the cmdline...
Is the truncated Chrome or Firefox thread comm really harmful or is
extending the task comm just for portable?
Could you pls show me some examples if the short comm is really harmful?
Per my understanding, if the short comm is harmful to applications
then it is worth extending it.
But if it is only for portable code, it may not be worth extending it.
[1]. https://github.com/freebsd/freebsd-src/blob/main/sys/sys/param.h#L126
Currently writes to /proc/PID/comm are silently truncated. This patch
makes the write() call return the actual number of bytes actually written
and on subsequent calls return -ENOSPC. glibc checks the length in
pthread_setname_np() before write(), so the change is not currently
relevant for it. I don't know/remember what other runtimes do, though.
This one I'm not really fond of because it abuses lseek() in that it
doesn't move the write pointer. But in case of /proc files this normally
would return EINVAL anyway.
Another possible way is introducing a new PR_GET_COMM_LEN for
prctl(2), but I'm not sure if it is worth it.
--
Thanks
Yafang
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: 2021-11-06 11:30:06
On Sat, Nov 06, 2021 at 05:12:24PM +0800, Yafang Shao wrote:
On Sat, Nov 6, 2021 at 7:57 AM Michał Mirosław [off-list ref] wrote:
quoted
On Fri, Nov 05, 2021 at 02:34:58PM +0800, Yafang Shao wrote:
quoted
On Thu, Nov 4, 2021 at 9:37 AM Michał Mirosław [off-list ref] wrote:
quoted
On Mon, Nov 01, 2021 at 06:04:08AM +0000, Yafang Shao wrote:
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.
This patchset tries to improve this problem fundamentally by extending
the task comm size from 16 to 24, which is a very simple way.
[...]
Hi,
I've tried something like this a few years back. My attempt got mostly
lost in the mailing lists, but I'm still carrying the patches in my
tree [1]. My target was userspace thread names, and it turned out more
involved than I had time for.
[1] https://rere.qmqm.pl/git/?p=linux;a=commit;h=2c3814268caf2b1fee6d1a0b61fd1730ce135d4a
and its parents
This was to enable longer thread names (as set by pthread_setname_np()).
Currently its 16 bytes, and that's too short for e.g. Chrome's or Firefox'es
threads. I believe that FreeBSD has 32-byte limit and so I expect that
major portable code is already prepared for bigger thread names.
The comm len in FreeBSD is (19 + 1) bytes[1], but that is still larger
than Linux :)
The task comm is short for many applications, that is why cmdline is
introduced per my understanding, but pthread_{set, get}name_np() is
reading/writing the comm or via prctl(2) rather than reading/writing
the cmdline...
Is the truncated Chrome or Firefox thread comm really harmful or is
extending the task comm just for portable?
Could you pls show me some examples if the short comm is really harmful?
Per my understanding, if the short comm is harmful to applications
then it is worth extending it.
But if it is only for portable code, it may not be worth extending it.
[1]. https://github.com/freebsd/freebsd-src/blob/main/sys/sys/param.h#L126
I don't think it is harmful as in exposing a bug or something. It's just
inconvenient when debugging a system where you can't differentiate
between threads because their names have been cut too short.
Best Regards
Michał Mirosław
From: Arnaldo Carvalho de Melo <acme@kernel.org> Date: 2021-11-17 14:31:08
Em Mon, Nov 01, 2021 at 06:04:16AM +0000, Yafang Shao escreveu:
quoted hunk
kernel test robot reported a perf-test failure after I extended task comm
size from 16 to 24. The failure as follows,
2021-10-13 18:00:46 sudo /usr/src/perf_selftests-x86_64-rhel-8.3-317419b91ef4eff4e2f046088201e4dc4065caa0/tools/perf/perf test 15
15: Parse sched tracepoints fields : FAILED!
The reason is perf-test requires a fixed-size task comm. If we extend
task comm size to 24, it will not equil with the required size 16 in perf
test.
After some analyzation, I found perf itself can adopt to the size
change, for example, below is the output of perf-sched after I extend
comm size to 24 -
task 614 ( kthreadd: 84), nr_events: 1
task 615 ( systemd: 843), nr_events: 1
task 616 ( networkd-dispat: 1026), nr_events: 1
task 617 ( systemd: 846), nr_events: 1
$ cat /proc/843/comm
networkd-dispatcher
The task comm can be displayed correctly as expected.
Replace old hard-coded 16 with the new one can fix the warning, but we'd
better make the test accept both old and new sizes, then it can be
backward compatibility.
After this patch, the perf-test succeeds no matter task comm is 16 or
24 -
15: Parse sched tracepoints fields : Ok
This patch is a preparation for the followup patch.
Reported-by: kernel test robot <redacted>
Suggested-by: Arnaldo Carvalho de Melo <redacted>
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---
tools/include/linux/sched.h | 11 +++++++++++
tools/perf/tests/evsel-tp-sched.c | 26 ++++++++++++++++++++------
2 files changed, 31 insertions(+), 6 deletions(-)
create mode 100644 tools/include/linux/sched.h
@@ -0,0 +1,11 @@+/* SPDX-License-Identifier: GPL-2.0 */+#ifndef _TOOLS_LINUX_SCHED_H+#define _TOOLS_LINUX_SCHED_H++/* Keep both length for backward compatibility */+enum{+TASK_COMM_LEN_16=16,+TASK_COMM_LEN=24,+};+
I don't think this is a good idea, to have it in tools/include/linux/,
we have /usr/include/linux/sched.h, this may end up confusing the build
at some point as your proposal is for a trimmed down header while what
is in /usr/include/linux/sched.h doesn't have just this.
But since we're using enums for this, we can't check for it with:
#ifdef TASK_COMM_LEN_16
#define TASK_COMM_LEN_16 16
#endif
ditto for TASK_COMM_LEN and be future proof, so I'd say just use
hardcoded values in tools/perf/tests/evsel-tp-sched.c?
- Arnaldo
@@ -23,15 +25,24 @@ static int evsel__test_field(struct evsel *evsel, const char *name, int size, boret=-1;}-if(field->size!=size){-pr_debug("%s: \"%s\" size (%d) should be %d!\n",+if(field->size!=size&&field->size!=alternate_size){+pr_debug("%s: \"%s\" size (%d) should be %d",evsel->name,name,field->size,size);+if(alternate_size>0)+pr_debug(" or %d",alternate_size);+pr_debug("!\n");ret=-1;}returnret;}+staticintevsel__test_field(structevsel*evsel,constchar*name,+intsize,boolshould_be_signed)+{+returnevsel__test_field_alt(evsel,name,size,-1,should_be_signed);+}+inttest__perf_evsel__tp_sched_test(structtest*test__maybe_unused,intsubtest__maybe_unused){structevsel*evsel=evsel__newtp("sched","sched_switch");
@@ -42,7 +53,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtesreturn-1;}-if(evsel__test_field(evsel,"prev_comm",16,false))+if(evsel__test_field_alt(evsel,"prev_comm",TASK_COMM_LEN_16,+TASK_COMM_LEN,false))ret=-1;if(evsel__test_field(evsel,"prev_pid",4,true))
@@ -54,7 +66,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtesif(evsel__test_field(evsel,"prev_state",sizeof(long),true))ret=-1;-if(evsel__test_field(evsel,"next_comm",16,false))+if(evsel__test_field_alt(evsel,"next_comm",TASK_COMM_LEN_16,+TASK_COMM_LEN,false))ret=-1;if(evsel__test_field(evsel,"next_pid",4,true))
@@ -72,7 +85,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtesreturn-1;}-if(evsel__test_field(evsel,"comm",16,false))+if(evsel__test_field_alt(evsel,"comm",TASK_COMM_LEN_16,+TASK_COMM_LEN,false))ret=-1;if(evsel__test_field(evsel,"pid",4,true))
On Wed, Nov 17, 2021 at 10:31 PM Arnaldo Carvalho de Melo
[off-list ref] wrote:
Em Mon, Nov 01, 2021 at 06:04:16AM +0000, Yafang Shao escreveu:
quoted
kernel test robot reported a perf-test failure after I extended task comm
size from 16 to 24. The failure as follows,
2021-10-13 18:00:46 sudo /usr/src/perf_selftests-x86_64-rhel-8.3-317419b91ef4eff4e2f046088201e4dc4065caa0/tools/perf/perf test 15
15: Parse sched tracepoints fields : FAILED!
The reason is perf-test requires a fixed-size task comm. If we extend
task comm size to 24, it will not equil with the required size 16 in perf
test.
After some analyzation, I found perf itself can adopt to the size
change, for example, below is the output of perf-sched after I extend
comm size to 24 -
task 614 ( kthreadd: 84), nr_events: 1
task 615 ( systemd: 843), nr_events: 1
task 616 ( networkd-dispat: 1026), nr_events: 1
task 617 ( systemd: 846), nr_events: 1
$ cat /proc/843/comm
networkd-dispatcher
The task comm can be displayed correctly as expected.
Replace old hard-coded 16 with the new one can fix the warning, but we'd
better make the test accept both old and new sizes, then it can be
backward compatibility.
After this patch, the perf-test succeeds no matter task comm is 16 or
24 -
15: Parse sched tracepoints fields : Ok
This patch is a preparation for the followup patch.
Reported-by: kernel test robot <redacted>
Suggested-by: Arnaldo Carvalho de Melo <redacted>
Signed-off-by: Yafang Shao <redacted>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <redacted>
Cc: Alexei Starovoitov <redacted>
Cc: Andrii Nakryiko <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <redacted>
Cc: Petr Mladek <pmladek@suse.com>
---
tools/include/linux/sched.h | 11 +++++++++++
tools/perf/tests/evsel-tp-sched.c | 26 ++++++++++++++++++++------
2 files changed, 31 insertions(+), 6 deletions(-)
create mode 100644 tools/include/linux/sched.h
@@ -0,0 +1,11 @@+/* SPDX-License-Identifier: GPL-2.0 */+#ifndef _TOOLS_LINUX_SCHED_H+#define _TOOLS_LINUX_SCHED_H++/* Keep both length for backward compatibility */+enum{+TASK_COMM_LEN_16=16,+TASK_COMM_LEN=24,+};+
I don't think this is a good idea, to have it in tools/include/linux/,
we have /usr/include/linux/sched.h, this may end up confusing the build
at some point as your proposal is for a trimmed down header while what
is in /usr/include/linux/sched.h doesn't have just this.
But since we're using enums for this, we can't check for it with:
#ifdef TASK_COMM_LEN_16
#define TASK_COMM_LEN_16 16
#endif
ditto for TASK_COMM_LEN and be future proof, so I'd say just use
hardcoded values in tools/perf/tests/evsel-tp-sched.c?
Hi Arnaldo,
Thanks for the review.
This perf tests code won't be changed in the latest version as we
don't want to extend comm size any more, see also
https://lore.kernel.org/lkml/20211108083840.4627-1-laoar.shao@gmail.com/
The hard-coded 16 in tools/perf/tests/evsel-tp-sched.c is kept as-is.
@@ -23,15 +25,24 @@ static int evsel__test_field(struct evsel *evsel, const char *name, int size, boret=-1;}-if(field->size!=size){-pr_debug("%s: \"%s\" size (%d) should be %d!\n",+if(field->size!=size&&field->size!=alternate_size){+pr_debug("%s: \"%s\" size (%d) should be %d",evsel->name,name,field->size,size);+if(alternate_size>0)+pr_debug(" or %d",alternate_size);+pr_debug("!\n");ret=-1;}returnret;}+staticintevsel__test_field(structevsel*evsel,constchar*name,+intsize,boolshould_be_signed)+{+returnevsel__test_field_alt(evsel,name,size,-1,should_be_signed);+}+inttest__perf_evsel__tp_sched_test(structtest*test__maybe_unused,intsubtest__maybe_unused){structevsel*evsel=evsel__newtp("sched","sched_switch");
@@ -42,7 +53,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtesreturn-1;}-if(evsel__test_field(evsel,"prev_comm",16,false))+if(evsel__test_field_alt(evsel,"prev_comm",TASK_COMM_LEN_16,+TASK_COMM_LEN,false))ret=-1;if(evsel__test_field(evsel,"prev_pid",4,true))
@@ -54,7 +66,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtesif(evsel__test_field(evsel,"prev_state",sizeof(long),true))ret=-1;-if(evsel__test_field(evsel,"next_comm",16,false))+if(evsel__test_field_alt(evsel,"next_comm",TASK_COMM_LEN_16,+TASK_COMM_LEN,false))ret=-1;if(evsel__test_field(evsel,"next_pid",4,true))
@@ -72,7 +85,8 @@ int test__perf_evsel__tp_sched_test(struct test *test __maybe_unused, int subtesreturn-1;}-if(evsel__test_field(evsel,"comm",16,false))+if(evsel__test_field_alt(evsel,"comm",TASK_COMM_LEN_16,+TASK_COMM_LEN,false))ret=-1;if(evsel__test_field(evsel,"pid",4,true))--