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
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 <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
Make sure the string set to task comm is always nul ternimated.
Signed-off-by: Yafang Shao <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Petr Mladek <[email protected]>
---
fs/exec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/exec.c b/fs/exec.c
index a098c133d8d7..404156b5b314 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1224,7 +1224,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
{
task_lock(tsk);
trace_task_rename(tsk, buf);
- strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+ strscpy_pad(tsk->comm, buf, sizeof(tsk->comm));
task_unlock(tsk);
perf_event_comm(tsk, exec);
}
--
2.17.1
If the dest buffer size is smaller than sizeof(tsk->comm), the buffer
will be without null ternimator, that may cause problem. 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 <[email protected]>
Suggested-by: Steven Rostedt <[email protected]>
Signed-off-by: Yafang Shao <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
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 <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Yafang Shao <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
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 <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Vladimir Zapolskiy <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: David Howells <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
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 <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
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 <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao <[email protected]> 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 <[email protected]> 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
On Fri, Oct 22, 2021 at 12:00 PM Kees Cook <[email protected]> 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 <[email protected]> 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