* [PATCH v5 01/15] fs/exec: make __set_task_comm always set a nul ternimated string
2021-10-21 3:45 [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Yafang Shao
@ 2021-10-21 3:45 ` Yafang Shao
2021-10-21 3:45 ` [PATCH v5 02/15] fs/exec: make __get_task_comm always get a nul terminated string Yafang Shao
` (8 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Yafang Shao @ 2021-10-21 3:45 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
Make sure the string set to task comm is always nul ternimated.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
fs/exec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/exec.c b/fs/exec.c
index a098c133d8d7..404156b5b314 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1224,7 +1224,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
{
task_lock(tsk);
trace_task_rename(tsk, buf);
- strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+ strscpy_pad(tsk->comm, buf, sizeof(tsk->comm));
task_unlock(tsk);
perf_event_comm(tsk, exec);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v5 02/15] fs/exec: make __get_task_comm always get a nul terminated string
2021-10-21 3:45 [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Yafang Shao
2021-10-21 3:45 ` [PATCH v5 01/15] fs/exec: make __set_task_comm always set a nul ternimated string Yafang Shao
@ 2021-10-21 3:45 ` Yafang Shao
2021-10-21 3:45 ` [PATCH v5 03/15] sched.h: introduce TASK_COMM_LEN_16 Yafang Shao
` (7 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Yafang Shao @ 2021-10-21 3:45 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
If the dest buffer size is smaller than sizeof(tsk->comm), the buffer
will be without null ternimator, that may cause problem. We can make sure
the buffer size not smaller than comm at the callsite to avoid that
problem, but there may be callsite that we can't easily change.
Using strscpy_pad() instead of strncpy() in __get_task_comm() can make
the string always nul ternimated.
Suggested-by: Kees Cook <keescook@chromium.org>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
fs/exec.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/exec.c b/fs/exec.c
index 404156b5b314..bf2a7a91eeea 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1209,7 +1209,8 @@ static int unshare_sighand(struct task_struct *me)
char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
{
task_lock(tsk);
- strncpy(buf, tsk->comm, buf_size);
+ /* The copied value is always null terminated */
+ strscpy_pad(buf, tsk->comm, buf_size);
task_unlock(tsk);
return buf;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v5 03/15] sched.h: introduce TASK_COMM_LEN_16
2021-10-21 3:45 [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Yafang Shao
2021-10-21 3:45 ` [PATCH v5 01/15] fs/exec: make __set_task_comm always set a nul ternimated string Yafang Shao
2021-10-21 3:45 ` [PATCH v5 02/15] fs/exec: make __get_task_comm always get a nul terminated string Yafang Shao
@ 2021-10-21 3:45 ` Yafang Shao
2021-10-21 21:55 ` Andrii Nakryiko
2021-10-21 3:45 ` [PATCH v5 04/15] cn_proc: make connector comm always nul ternimated Yafang Shao
` (6 subsequent siblings)
9 siblings, 1 reply; 15+ messages in thread
From: Yafang Shao @ 2021-10-21 3:45 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
There're many hard-coded 16 used to store task comm in the kernel, that
makes it error prone if we want to change the value of TASK_COMM_LEN. A
new marco TASK_COMM_LEN_16 is introduced to replace these old ones, then
we can easily grep them.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
include/linux/sched.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index c1a927ddec64..62d5b30d310c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -274,6 +274,8 @@ struct task_group;
#define get_current_state() READ_ONCE(current->__state)
+/* To replace the old hard-coded 16 */
+#define TASK_COMM_LEN_16 16
/* Task command name length: */
#define TASK_COMM_LEN 16
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v5 03/15] sched.h: introduce TASK_COMM_LEN_16
2021-10-21 3:45 ` [PATCH v5 03/15] sched.h: introduce TASK_COMM_LEN_16 Yafang Shao
@ 2021-10-21 21:55 ` Andrii Nakryiko
2021-10-22 6:23 ` Yafang Shao
0 siblings, 1 reply; 15+ messages in thread
From: Andrii Nakryiko @ 2021-10-21 21:55 UTC (permalink / raw)
To: Yafang Shao
Cc: Kees Cook, Steven Rostedt, Mathieu Desnoyers,
Arnaldo Carvalho de Melo, Petr Mladek, Peter Ziljstra, Al Viro,
Andrew Morton, Valentin Schneider, qiang.zhang, robdclark,
Christian Brauner, Dietmar Eggemann, Ingo Molnar, juri.lelli,
Vincent Guittot, David S. Miller, Jakub Kicinski,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, Networking,
bpf, linux-perf-use.,
linux-fsdevel, open list, oliver.sang, kbuild test robot
On Wed, Oct 20, 2021 at 8:45 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> There're many hard-coded 16 used to store task comm in the kernel, that
> makes it error prone if we want to change the value of TASK_COMM_LEN. A
> new marco TASK_COMM_LEN_16 is introduced to replace these old ones, then
> we can easily grep them.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Petr Mladek <pmladek@suse.com>
> ---
> include/linux/sched.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index c1a927ddec64..62d5b30d310c 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -274,6 +274,8 @@ struct task_group;
>
> #define get_current_state() READ_ONCE(current->__state)
>
> +/* To replace the old hard-coded 16 */
> +#define TASK_COMM_LEN_16 16
> /* Task command name length: */
> #define TASK_COMM_LEN 16
Can we please convert these two constants into enum? That will allow
BPF applications to deal with such kernel change more easily because
these constants will now be available as part of kernel BTF.
Something like this should be completely equivalent for all the kernel uses:
enum {
TASK_COMM_LEN = 16,
TASK_COMM_LEN_16 = 16,
};
When later TASK_COMM_LEN is defined as = 24, BPF applications will be
able to deal with that by querying BTF through BPF CO-RE.
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 03/15] sched.h: introduce TASK_COMM_LEN_16
2021-10-21 21:55 ` Andrii Nakryiko
@ 2021-10-22 6:23 ` Yafang Shao
0 siblings, 0 replies; 15+ messages in thread
From: Yafang Shao @ 2021-10-22 6:23 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Kees Cook, Steven Rostedt, Mathieu Desnoyers,
Arnaldo Carvalho de Melo, Petr Mladek, Peter Ziljstra, Al Viro,
Andrew Morton, Valentin Schneider, Qiang Zhang, robdclark,
Christian Brauner, Dietmar Eggemann, Ingo Molnar, Juri Lelli,
Vincent Guittot, David S. Miller, Jakub Kicinski,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, Networking,
bpf, linux-perf-use.,
linux-fsdevel, open list, kernel test robot, kbuild test robot
On Fri, Oct 22, 2021 at 5:55 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Oct 20, 2021 at 8:45 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > There're many hard-coded 16 used to store task comm in the kernel, that
> > makes it error prone if we want to change the value of TASK_COMM_LEN. A
> > new marco TASK_COMM_LEN_16 is introduced to replace these old ones, then
> > we can easily grep them.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Petr Mladek <pmladek@suse.com>
> > ---
> > include/linux/sched.h | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index c1a927ddec64..62d5b30d310c 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -274,6 +274,8 @@ struct task_group;
> >
> > #define get_current_state() READ_ONCE(current->__state)
> >
> > +/* To replace the old hard-coded 16 */
> > +#define TASK_COMM_LEN_16 16
> > /* Task command name length: */
> > #define TASK_COMM_LEN 16
>
> Can we please convert these two constants into enum? That will allow
> BPF applications to deal with such kernel change more easily because
> these constants will now be available as part of kernel BTF.
>
> Something like this should be completely equivalent for all the kernel uses:
>
> enum {
> TASK_COMM_LEN = 16,
> TASK_COMM_LEN_16 = 16,
> };
>
> When later TASK_COMM_LEN is defined as = 24, BPF applications will be
> able to deal with that by querying BTF through BPF CO-RE.
>
Sure. I will convert it to enum.
--
Thanks
Yafang
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 04/15] cn_proc: make connector comm always nul ternimated
2021-10-21 3:45 [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Yafang Shao
` (2 preceding siblings ...)
2021-10-21 3:45 ` [PATCH v5 03/15] sched.h: introduce TASK_COMM_LEN_16 Yafang Shao
@ 2021-10-21 3:45 ` Yafang Shao
2021-10-21 3:45 ` [PATCH v5 05/15] drivers/infiniband: make setup_ctxt always get a nul terminated task comm Yafang Shao
` (5 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Yafang Shao @ 2021-10-21 3:45 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao, Vladimir Zapolskiy, David Howells
connector comm was introduced in commit
f786ecba4158 ("connector: add comm change event report to proc connector").
struct comm_proc_event was defined in include/linux/cn_proc.h first and
then been moved into file include/uapi/linux/cn_proc.h in commit
607ca46e97a1 ("UAPI: (Scripted) Disintegrate include/linux").
As this is the UAPI code, we can't change it without potentially breaking
things (i.e. userspace binaries have this size built in, so we can't just
change the size). To prepare for the followup change - extending task
comm, we have to use __get_task_comm() to avoid the BUILD_BUG_ON() in
proc_comm_connector().
__get_task_comm() always get a nul terminated string, so we don't worry
about whether it is truncated or not.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Vladimir Zapolskiy <vzapolskiy@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
drivers/connector/cn_proc.c | 5 ++++-
include/uapi/linux/cn_proc.h | 7 ++++++-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 646ad385e490..c88ba2dc1eae 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -230,7 +230,10 @@ void proc_comm_connector(struct task_struct *task)
ev->what = PROC_EVENT_COMM;
ev->event_data.comm.process_pid = task->pid;
ev->event_data.comm.process_tgid = task->tgid;
- get_task_comm(ev->event_data.comm.comm, task);
+
+ /* This may get truncated. */
+ __get_task_comm(ev->event_data.comm.comm,
+ sizeof(ev->event_data.comm.comm), task);
memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
msg->ack = 0; /* not used */
diff --git a/include/uapi/linux/cn_proc.h b/include/uapi/linux/cn_proc.h
index db210625cee8..4bb7f658fcc0 100644
--- a/include/uapi/linux/cn_proc.h
+++ b/include/uapi/linux/cn_proc.h
@@ -21,6 +21,11 @@
#include <linux/types.h>
+/* We can't include <linux/sched.h> directly in this UAPI header. */
+#ifndef TASK_COMM_LEN_16
+#define TASK_COMM_LEN_16 16
+#endif
+
/*
* Userspace sends this enum to register with the kernel that it is listening
* for events on the connector.
@@ -110,7 +115,7 @@ struct proc_event {
struct comm_proc_event {
__kernel_pid_t process_pid;
__kernel_pid_t process_tgid;
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
} comm;
struct coredump_proc_event {
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v5 05/15] drivers/infiniband: make setup_ctxt always get a nul terminated task comm
2021-10-21 3:45 [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Yafang Shao
` (3 preceding siblings ...)
2021-10-21 3:45 ` [PATCH v5 04/15] cn_proc: make connector comm always nul ternimated Yafang Shao
@ 2021-10-21 3:45 ` Yafang Shao
2021-10-21 3:45 ` [PATCH v5 06/15] elfcore: make prpsinfo " Yafang Shao
` (4 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Yafang Shao @ 2021-10-21 3:45 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
Use TASK_COMM_LEN_16 instead of hard-coded 16 to make it more grepable,
and use strscpy_pad() instead of strlcpy() to make the comm always nul
terminated.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
drivers/infiniband/hw/qib/qib.h | 4 ++--
drivers/infiniband/hw/qib/qib_file_ops.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/infiniband/hw/qib/qib.h b/drivers/infiniband/hw/qib/qib.h
index 9363bccfc6e7..8e59f9cbabc8 100644
--- a/drivers/infiniband/hw/qib/qib.h
+++ b/drivers/infiniband/hw/qib/qib.h
@@ -195,8 +195,8 @@ struct qib_ctxtdata {
/* pid of process using this ctxt */
pid_t pid;
pid_t subpid[QLOGIC_IB_MAX_SUBCTXT];
- /* same size as task_struct .comm[], command that opened context */
- char comm[16];
+ /* task_struct .comm[], command that opened context */
+ char comm[TASK_COMM_LEN_16];
/* pkeys set by this use of this ctxt */
u16 pkeys[4];
/* so file ops can get at unit */
diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
index 63854f4b6524..7ab2b448c183 100644
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -1321,7 +1321,7 @@ static int setup_ctxt(struct qib_pportdata *ppd, int ctxt,
rcd->tid_pg_list = ptmp;
rcd->pid = current->pid;
init_waitqueue_head(&dd->rcd[ctxt]->wait);
- strlcpy(rcd->comm, current->comm, sizeof(rcd->comm));
+ strscpy_pad(rcd->comm, current->comm, sizeof(rcd->comm));
ctxt_fp(fp) = rcd;
qib_stats.sps_ctxts++;
dd->freectxts--;
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v5 06/15] elfcore: make prpsinfo always get a nul terminated task comm
2021-10-21 3:45 [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Yafang Shao
` (4 preceding siblings ...)
2021-10-21 3:45 ` [PATCH v5 05/15] drivers/infiniband: make setup_ctxt always get a nul terminated task comm Yafang Shao
@ 2021-10-21 3:45 ` Yafang Shao
2021-10-21 3:45 ` [PATCH v5 07/15] samples/bpf/kern: use TASK_COMM_LEN instead of hard-coded 16 Yafang Shao
` (3 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Yafang Shao @ 2021-10-21 3:45 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
kernel test robot reported a -Wstringop-truncation warning after I
extend task comm from 16 to 24. Below is the detailed warning:
fs/binfmt_elf.c: In function 'fill_psinfo.isra':
>> fs/binfmt_elf.c:1575:9: warning: 'strncpy' output may be truncated copying 16 bytes from a string of length 23 [-Wstringop-truncation]
1575 | strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This patch can fix this warning.
struct elf_prpsinfo is used to dump the task information in userspace
coredump or kernel vmcore. Use TASK_COMM_LEN_16 instead of hard-coded 16
to make it more grepable, and use strscpy_pad() instead of strncpy() to
make it always get a nul terminated task comm.
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
fs/binfmt_elf.c | 2 +-
include/linux/elfcore-compat.h | 3 ++-
include/linux/elfcore.h | 4 ++--
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index a813b70f594e..a4ba79fce2a9 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1572,7 +1572,7 @@ static int fill_psinfo(struct elf_prpsinfo *psinfo, struct task_struct *p,
SET_UID(psinfo->pr_uid, from_kuid_munged(cred->user_ns, cred->uid));
SET_GID(psinfo->pr_gid, from_kgid_munged(cred->user_ns, cred->gid));
rcu_read_unlock();
- strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
+ strscpy_pad(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
return 0;
}
diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h
index e272c3d452ce..69fa1a728964 100644
--- a/include/linux/elfcore-compat.h
+++ b/include/linux/elfcore-compat.h
@@ -5,6 +5,7 @@
#include <linux/elf.h>
#include <linux/elfcore.h>
#include <linux/compat.h>
+#include <linux/sched.h>
/*
* Make sure these layouts match the linux/elfcore.h native definitions.
@@ -43,7 +44,7 @@ struct compat_elf_prpsinfo
__compat_uid_t pr_uid;
__compat_gid_t pr_gid;
compat_pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
- char pr_fname[16];
+ char pr_fname[TASK_COMM_LEN_16];
char pr_psargs[ELF_PRARGSZ];
};
diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h
index 2aaa15779d50..ee7ac09734ba 100644
--- a/include/linux/elfcore.h
+++ b/include/linux/elfcore.h
@@ -65,8 +65,8 @@ struct elf_prpsinfo
__kernel_gid_t pr_gid;
pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
/* Lots missing */
- char pr_fname[16]; /* filename of executable */
- char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
+ char pr_fname[TASK_COMM_LEN_16]; /* filename of executable */
+ char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
};
static inline void elf_core_copy_regs(elf_gregset_t *elfregs, struct pt_regs *regs)
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v5 07/15] samples/bpf/kern: use TASK_COMM_LEN instead of hard-coded 16
2021-10-21 3:45 [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Yafang Shao
` (5 preceding siblings ...)
2021-10-21 3:45 ` [PATCH v5 06/15] elfcore: make prpsinfo " Yafang Shao
@ 2021-10-21 3:45 ` Yafang Shao
2021-10-21 3:45 ` [PATCH v5 08/15] samples/bpf/user: use TASK_COMM_LEN_16 " Yafang Shao
` (2 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Yafang Shao @ 2021-10-21 3:45 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
The linux/sched.h is visible to the bpf kernel modules, so we can use
TASK_COMM_LEN_16 to replace the hard-coded 16 in these bpf kernel
modules to make it more grepable.
In these bpf modules, someone gets task comm via bpf_get_current_comm(),
which always get a nul terminated string. While someone gets task comm via
bpf_probe_read_kernel(), which is unsafe, we should use
bpf_probe_read_kernel_str() instead.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
samples/bpf/offwaketime_kern.c | 10 +++++-----
samples/bpf/test_overhead_kprobe_kern.c | 11 ++++++-----
samples/bpf/test_overhead_tp_kern.c | 5 +++--
samples/bpf/tracex2_kern.c | 3 ++-
4 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/samples/bpf/offwaketime_kern.c b/samples/bpf/offwaketime_kern.c
index 4866afd054da..c0fd04497eea 100644
--- a/samples/bpf/offwaketime_kern.c
+++ b/samples/bpf/offwaketime_kern.c
@@ -23,8 +23,8 @@
#define MAX_ENTRIES 10000
struct key_t {
- char waker[TASK_COMM_LEN];
- char target[TASK_COMM_LEN];
+ char waker[TASK_COMM_LEN_16];
+ char target[TASK_COMM_LEN_16];
u32 wret;
u32 tret;
};
@@ -44,7 +44,7 @@ struct {
} start SEC(".maps");
struct wokeby_t {
- char name[TASK_COMM_LEN];
+ char name[TASK_COMM_LEN_16];
u32 ret;
};
@@ -113,11 +113,11 @@ static inline int update_counts(void *ctx, u32 pid, u64 delta)
/* taken from /sys/kernel/debug/tracing/events/sched/sched_switch/format */
struct sched_switch_args {
unsigned long long pad;
- char prev_comm[16];
+ char prev_comm[TASK_COMM_LEN_16];
int prev_pid;
int prev_prio;
long long prev_state;
- char next_comm[16];
+ char next_comm[TASK_COMM_LEN_16];
int next_pid;
int next_prio;
};
diff --git a/samples/bpf/test_overhead_kprobe_kern.c b/samples/bpf/test_overhead_kprobe_kern.c
index f6d593e47037..31e8c5ee0cdc 100644
--- a/samples/bpf/test_overhead_kprobe_kern.c
+++ b/samples/bpf/test_overhead_kprobe_kern.c
@@ -6,6 +6,7 @@
*/
#include <linux/version.h>
#include <linux/ptrace.h>
+#include <linux/sched.h>
#include <uapi/linux/bpf.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
@@ -22,17 +23,17 @@ int prog(struct pt_regs *ctx)
{
struct signal_struct *signal;
struct task_struct *tsk;
- char oldcomm[16] = {};
- char newcomm[16] = {};
+ char oldcomm[TASK_COMM_LEN_16] = {};
+ char newcomm[TASK_COMM_LEN_16] = {};
u16 oom_score_adj;
u32 pid;
tsk = (void *)PT_REGS_PARM1(ctx);
pid = _(tsk->pid);
- bpf_probe_read_kernel(oldcomm, sizeof(oldcomm), &tsk->comm);
- bpf_probe_read_kernel(newcomm, sizeof(newcomm),
- (void *)PT_REGS_PARM2(ctx));
+ bpf_probe_read_kernel_str(oldcomm, sizeof(oldcomm), &tsk->comm);
+ bpf_probe_read_kernel_str(newcomm, sizeof(newcomm),
+ (void *)PT_REGS_PARM2(ctx));
signal = _(tsk->signal);
oom_score_adj = _(signal->oom_score_adj);
return 0;
diff --git a/samples/bpf/test_overhead_tp_kern.c b/samples/bpf/test_overhead_tp_kern.c
index eaa32693f8fc..a6d5b3301af2 100644
--- a/samples/bpf/test_overhead_tp_kern.c
+++ b/samples/bpf/test_overhead_tp_kern.c
@@ -4,6 +4,7 @@
* modify it under the terms of version 2 of the GNU General Public
* License as published by the Free Software Foundation.
*/
+#include <linux/sched.h>
#include <uapi/linux/bpf.h>
#include <bpf/bpf_helpers.h>
@@ -11,8 +12,8 @@
struct task_rename {
__u64 pad;
__u32 pid;
- char oldcomm[16];
- char newcomm[16];
+ char oldcomm[TASK_COMM_LEN_16];
+ char newcomm[TASK_COMM_LEN_16];
__u16 oom_score_adj;
};
SEC("tracepoint/task/task_rename")
diff --git a/samples/bpf/tracex2_kern.c b/samples/bpf/tracex2_kern.c
index 5bc696bac27d..d70ce59055cb 100644
--- a/samples/bpf/tracex2_kern.c
+++ b/samples/bpf/tracex2_kern.c
@@ -7,6 +7,7 @@
#include <linux/skbuff.h>
#include <linux/netdevice.h>
#include <linux/version.h>
+#include <linux/sched.h>
#include <uapi/linux/bpf.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
@@ -65,7 +66,7 @@ static unsigned int log2l(unsigned long v)
}
struct hist_key {
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
u64 pid_tgid;
u64 uid_gid;
u64 index;
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v5 08/15] samples/bpf/user: use TASK_COMM_LEN_16 instead of hard-coded 16
2021-10-21 3:45 [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Yafang Shao
` (6 preceding siblings ...)
2021-10-21 3:45 ` [PATCH v5 07/15] samples/bpf/kern: use TASK_COMM_LEN instead of hard-coded 16 Yafang Shao
@ 2021-10-21 3:45 ` Yafang Shao
2021-10-21 3:45 ` [PATCH v5 09/15] tools/include: introduce TASK_COMM_LEN_16 Yafang Shao
2021-10-22 3:52 ` [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Andrew Morton
9 siblings, 0 replies; 15+ messages in thread
From: Yafang Shao @ 2021-10-21 3:45 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
The task comm size is invisible to the bpf userspace, we have to
define a new TASK_COMM_LEN_16 in the userspace. Use this macro instead
of the hard-coded 16 can make it more grepable.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
samples/bpf/offwaketime_user.c | 6 +++---
samples/bpf/tracex2_user.c | 7 ++++---
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/samples/bpf/offwaketime_user.c b/samples/bpf/offwaketime_user.c
index 73a986876c1a..ca918ac93ee7 100644
--- a/samples/bpf/offwaketime_user.c
+++ b/samples/bpf/offwaketime_user.c
@@ -36,11 +36,11 @@ static void print_ksym(__u64 addr)
printf("%s;", sym->name);
}
-#define TASK_COMM_LEN 16
+#define TASK_COMM_LEN_16 16
struct key_t {
- char waker[TASK_COMM_LEN];
- char target[TASK_COMM_LEN];
+ char waker[TASK_COMM_LEN_16];
+ char target[TASK_COMM_LEN_16];
__u32 wret;
__u32 tret;
};
diff --git a/samples/bpf/tracex2_user.c b/samples/bpf/tracex2_user.c
index 1626d51dfffd..70081d917c6d 100644
--- a/samples/bpf/tracex2_user.c
+++ b/samples/bpf/tracex2_user.c
@@ -10,8 +10,9 @@
#include <bpf/libbpf.h>
#include "bpf_util.h"
-#define MAX_INDEX 64
-#define MAX_STARS 38
+#define MAX_INDEX 64
+#define MAX_STARS 38
+#define TASK_COMM_LEN_16 16
/* my_map, my_hist_map */
static int map_fd[2];
@@ -28,7 +29,7 @@ static void stars(char *str, long val, long max, int width)
}
struct task {
- char comm[16];
+ char comm[TASK_COMM_LEN_16];
__u64 pid_tgid;
__u64 uid_gid;
};
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v5 09/15] tools/include: introduce TASK_COMM_LEN_16
2021-10-21 3:45 [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Yafang Shao
` (7 preceding siblings ...)
2021-10-21 3:45 ` [PATCH v5 08/15] samples/bpf/user: use TASK_COMM_LEN_16 " Yafang Shao
@ 2021-10-21 3:45 ` Yafang Shao
2021-10-22 3:52 ` [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Andrew Morton
9 siblings, 0 replies; 15+ messages in thread
From: Yafang Shao @ 2021-10-21 3:45 UTC (permalink / raw)
To: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, akpm, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh
Cc: netdev, bpf, linux-perf-users, linux-fsdevel, linux-kernel,
oliver.sang, lkp, Yafang Shao
TASK_COMM_LEN_16 is introduced to replace all the hard-coded 16 used in
the files under tools/ directory.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
tools/include/linux/sched/task.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/include/linux/sched/task.h b/tools/include/linux/sched/task.h
index a97890eca110..7657dd3e0e02 100644
--- a/tools/include/linux/sched/task.h
+++ b/tools/include/linux/sched/task.h
@@ -1,4 +1,6 @@
#ifndef _TOOLS_PERF_LINUX_SCHED_TASK_H
#define _TOOLS_PERF_LINUX_SCHED_TASK_H
+#define TASK_COMM_LEN_16 16
+
#endif /* _TOOLS_PERF_LINUX_SCHED_TASK_H */
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL
2021-10-21 3:45 [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Yafang Shao
` (8 preceding siblings ...)
2021-10-21 3:45 ` [PATCH v5 09/15] tools/include: introduce TASK_COMM_LEN_16 Yafang Shao
@ 2021-10-22 3:52 ` Andrew Morton
2021-10-22 4:00 ` Kees Cook
9 siblings, 1 reply; 15+ messages in thread
From: Andrew Morton @ 2021-10-22 3:52 UTC (permalink / raw)
To: Yafang Shao
Cc: keescook, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh, netdev, bpf, linux-perf-users,
linux-fsdevel, linux-kernel, oliver.sang, lkp
On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao <laoar.shao@gmail.com> wrote:
> This patchset changes files among many subsystems. I don't know which
> tree it should be applied to, so I just base it on Linus's tree.
I can do that ;)
> There're many truncated kthreads in the kernel, which may make trouble
> for the user, for example, the user can't get detailed device
> information from the task comm.
That sucked of us.
> This patchset tries to improve this problem fundamentally by extending
> the task comm size from 16 to 24. In order to do that, we have to do
> some cleanups first.
It's at v5 and there's no evidence of review activity? C'mon, folks!
> 1. Make the copy of task comm always safe no matter what the task
> comm size is. For example,
>
> Unsafe Safe
> strlcpy strscpy_pad
> strncpy strscpy_pad
> bpf_probe_read_kernel bpf_probe_read_kernel_str
> bpf_core_read_str
> bpf_get_current_comm
> perf_event__prepare_comm
> prctl(2)
>
> 2. Replace the old hard-coded 16 with a new macro TASK_COMM_LEN_16 to
> make it more grepable.
>
> 3. Extend the task comm size to 24 for CONFIG_BASE_FULL case and keep it
> as 16 for CONFIG_BASE_SMALL.
Is this justified? How much simpler/more reliable/more maintainable/
would the code be if we were to make CONFIG_BASE_SMALL suffer with the
extra 8 bytes?
> 4. Print a warning if the kthread comm is still truncated.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL
2021-10-22 3:52 ` [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL Andrew Morton
@ 2021-10-22 4:00 ` Kees Cook
2021-10-22 6:20 ` Yafang Shao
0 siblings, 1 reply; 15+ messages in thread
From: Kees Cook @ 2021-10-22 4:00 UTC (permalink / raw)
To: Andrew Morton
Cc: Yafang Shao, rostedt, mathieu.desnoyers, arnaldo.melo, pmladek,
peterz, viro, valentin.schneider, qiang.zhang, robdclark,
christian, dietmar.eggemann, mingo, juri.lelli, vincent.guittot,
davem, kuba, ast, daniel, andrii, kafai, songliubraving, yhs,
john.fastabend, kpsingh, netdev, bpf, linux-perf-users,
linux-fsdevel, linux-kernel, oliver.sang, lkp
On Thu, Oct 21, 2021 at 08:52:22PM -0700, Andrew Morton wrote:
> On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao <laoar.shao@gmail.com> wrote:
>
> > This patchset changes files among many subsystems. I don't know which
> > tree it should be applied to, so I just base it on Linus's tree.
>
> I can do that ;)
>
> > There're many truncated kthreads in the kernel, which may make trouble
> > for the user, for example, the user can't get detailed device
> > information from the task comm.
>
> That sucked of us.
>
> > This patchset tries to improve this problem fundamentally by extending
> > the task comm size from 16 to 24. In order to do that, we have to do
> > some cleanups first.
>
> It's at v5 and there's no evidence of review activity? C'mon, folks!
It's on my list! :) It's a pretty subtle area that rarely changes, so I
want to make sure I'm a full coffee to do the review. :)
> > 1. Make the copy of task comm always safe no matter what the task
> > comm size is. For example,
> >
> > Unsafe Safe
> > strlcpy strscpy_pad
> > strncpy strscpy_pad
> > bpf_probe_read_kernel bpf_probe_read_kernel_str
> > bpf_core_read_str
> > bpf_get_current_comm
> > perf_event__prepare_comm
> > prctl(2)
> >
> > 2. Replace the old hard-coded 16 with a new macro TASK_COMM_LEN_16 to
> > make it more grepable.
> >
> > 3. Extend the task comm size to 24 for CONFIG_BASE_FULL case and keep it
> > as 16 for CONFIG_BASE_SMALL.
>
> Is this justified? How much simpler/more reliable/more maintainable/
> would the code be if we were to make CONFIG_BASE_SMALL suffer with the
> extra 8 bytes?
Does anyone "own" CONFIG_BASE_SMALL? Gonna go with "no":
$ git ann init/Kconfig| grep 'config BASE_SMALL'
1da177e4c3f41 (Linus Torvalds 2005-04-16 15:20:36 -0700 2054)config BASE_SMALL
And it looks mostly unused:
$ git grep CONFIG_BASE_SMALL | cut -d: -f1 | sort -u | xargs -n1 git ann -f | grep 'CONFIG_BASE_SMALL'
b2af018ff26f1 (Ingo Molnar 2009-01-28 17:36:56 +0100 18)#if CONFIG_BASE_SMALL == 0
fcdba07ee390d ( Jiri Olsa 2011-02-07 19:31:25 +0100 54)#define CON_BUF_SIZE (CONFIG_BASE_SMALL ? 256 : PAGE_SIZE)
Blaming lines: 100% (46/46), done.
1da177e4c3f41 (Linus Torvalds 2005-04-16 15:20:36 -0700 28)#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000)
1da177e4c3f41 (Linus Torvalds 2005-04-16 15:20:36 -0700 34)#define PID_MAX_LIMIT (CONFIG_BASE_SMALL ? PAGE_SIZE * 8 : \
Blaming lines: 100% (162/162), done.
f86dcc5aa8c79 (Eric Dumazet 2009-10-07 00:37:59 +0000 31)#define UDP_HTABLE_SIZE_MIN (CONFIG_BASE_SMALL ? 128 : 256)
02c02bf12c5d8 (Matthew Wilcox 2017-11-03 23:09:45 -0400 1110)#define XA_CHUNK_SHIFT (CONFIG_BASE_SMALL ? 4 : 6)
a52b89ebb6d44 (Davidlohr Bueso 2014-01-12 15:31:23 -0800 4249)#if CONFIG_BASE_SMALL
7b44ab978b77a (Eric W. Biederman 2011-11-16 23:20:58 -0800 78)#define UIDHASH_BITS (CONFIG_BASE_SMALL ? 3 : 7)
--
Kees Cook
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 00/15] extend task comm from 16 to 24 for CONFIG_BASE_FULL
2021-10-22 4:00 ` Kees Cook
@ 2021-10-22 6:20 ` Yafang Shao
0 siblings, 0 replies; 15+ messages in thread
From: Yafang Shao @ 2021-10-22 6:20 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Steven Rostedt, Mathieu Desnoyers,
Arnaldo Carvalho de Melo, Petr Mladek, Peter Zijlstra, Al Viro,
Valentin Schneider, Qiang Zhang, robdclark, christian,
Dietmar Eggemann, Ingo Molnar, Juri Lelli, Vincent Guittot,
David Miller, kuba, Alexei Starovoitov, Daniel Borkmann, andrii,
kafai, Song Liu, Yonghong Song, john.fastabend, kpsingh, netdev,
bpf, linux-perf-users, linux-fsdevel, LKML, kernel test robot,
kbuild test robot
On Fri, Oct 22, 2021 at 12:00 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Oct 21, 2021 at 08:52:22PM -0700, Andrew Morton wrote:
> > On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > > This patchset changes files among many subsystems. I don't know which
> > > tree it should be applied to, so I just base it on Linus's tree.
> >
> > I can do that ;)
> >
> > > There're many truncated kthreads in the kernel, which may make trouble
> > > for the user, for example, the user can't get detailed device
> > > information from the task comm.
> >
> > That sucked of us.
> >
> > > This patchset tries to improve this problem fundamentally by extending
> > > the task comm size from 16 to 24. In order to do that, we have to do
> > > some cleanups first.
> >
> > It's at v5 and there's no evidence of review activity? C'mon, folks!
>
> It's on my list! :) It's a pretty subtle area that rarely changes, so I
> want to make sure I'm a full coffee to do the review. :)
>
> > > 1. Make the copy of task comm always safe no matter what the task
> > > comm size is. For example,
> > >
> > > Unsafe Safe
> > > strlcpy strscpy_pad
> > > strncpy strscpy_pad
> > > bpf_probe_read_kernel bpf_probe_read_kernel_str
> > > bpf_core_read_str
> > > bpf_get_current_comm
> > > perf_event__prepare_comm
> > > prctl(2)
> > >
> > > 2. Replace the old hard-coded 16 with a new macro TASK_COMM_LEN_16 to
> > > make it more grepable.
> > >
> > > 3. Extend the task comm size to 24 for CONFIG_BASE_FULL case and keep it
> > > as 16 for CONFIG_BASE_SMALL.
> >
> > Is this justified? How much simpler/more reliable/more maintainable/
> > would the code be if we were to make CONFIG_BASE_SMALL suffer with the
> > extra 8 bytes?
>
> Does anyone "own" CONFIG_BASE_SMALL? Gonna go with "no":
>
> $ git ann init/Kconfig| grep 'config BASE_SMALL'
> 1da177e4c3f41 (Linus Torvalds 2005-04-16 15:20:36 -0700 2054)config BASE_SMALL
>
> And it looks mostly unused:
>
> $ git grep CONFIG_BASE_SMALL | cut -d: -f1 | sort -u | xargs -n1 git ann -f | grep 'CONFIG_BASE_SMALL'
> b2af018ff26f1 (Ingo Molnar 2009-01-28 17:36:56 +0100 18)#if CONFIG_BASE_SMALL == 0
> fcdba07ee390d ( Jiri Olsa 2011-02-07 19:31:25 +0100 54)#define CON_BUF_SIZE (CONFIG_BASE_SMALL ? 256 : PAGE_SIZE)
> Blaming lines: 100% (46/46), done.
> 1da177e4c3f41 (Linus Torvalds 2005-04-16 15:20:36 -0700 28)#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000)
> 1da177e4c3f41 (Linus Torvalds 2005-04-16 15:20:36 -0700 34)#define PID_MAX_LIMIT (CONFIG_BASE_SMALL ? PAGE_SIZE * 8 : \
> Blaming lines: 100% (162/162), done.
> f86dcc5aa8c79 (Eric Dumazet 2009-10-07 00:37:59 +0000 31)#define UDP_HTABLE_SIZE_MIN (CONFIG_BASE_SMALL ? 128 : 256)
> 02c02bf12c5d8 (Matthew Wilcox 2017-11-03 23:09:45 -0400 1110)#define XA_CHUNK_SHIFT (CONFIG_BASE_SMALL ? 4 : 6)
> a52b89ebb6d44 (Davidlohr Bueso 2014-01-12 15:31:23 -0800 4249)#if CONFIG_BASE_SMALL
> 7b44ab978b77a (Eric W. Biederman 2011-11-16 23:20:58 -0800 78)#define UIDHASH_BITS (CONFIG_BASE_SMALL ? 3 : 7)
>
>
> --
Right. CONFIG_BASE_SMALL is seldomly used in the kernel.
As you have already removed 64 bytes from task_struct, I think we can
extend the 8 bytes for CONFIG_BASE_SMALL as well.
--
Thanks
Yafang
^ permalink raw reply [flat|nested] 15+ messages in thread