[PATCH v2 0/4] task_struct: extend task comm from 16 to 24 for CONFIG_BASE_FULL

[PATCH v2 0/4] task_struct: extend task comm from 16 to 24 for CONFIG_BASE_FULL

[Yafang Shao]

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-
    nvidia-modeset/

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-
    ...

Besides the in-tree kthreads listed above, the out-of-tree kthreads may
also be truncated:

    rtase_work_queu
    nvidia-modeset/
    UVM global queu
    UVM deferred re
    ...

We should improve this problem fundamentally.

This patch extends the size of task comm to 24 bytes, which is the
same length with workqueue's, for the CONFIG_BASE_FULL case. And for the
CONFIG_BASE_SMALL case, the size of task comm is still kept as 16 bytes.

If the kthread's comm is still truncated, a warning will be printed.
Below is the result of my test case:

truncated kthread comm:I-am-a-kthread-with-lon, pid:14 by 6 characters

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

Yafang Shao (4):
  cn_proc.h: use TASK_COMM_LEN instread of 16 in struct proc_event
  fs/exec: use strscpy instead of strlcpy in __set_task_comm
  sched.h: extend task comm from 16 to 24 for CONFIG_BASE_FULL
  kernel/kthread: show a warning if kthread's comm is truncated

 fs/exec.c                    | 2 +-
 include/linux/sched.h        | 4 ++++
 include/uapi/linux/cn_proc.h | 2 +-
 kernel/kthread.c             | 7 ++++++-
 4 files changed, 12 insertions(+), 3 deletions(-)

-- 
2.18.2

[PATCH v2 1/4] cn_proc.h: use TASK_COMM_LEN instread of 16 in struct proc_event

[Yafang Shao]

struct comm_proc_event was introduced in commit
f786ecba4158 ("connector: add comm change event report to proc connector").
It seems that there is no strong reason we must define the comm as a
hardcode 16 bytes. So we can use TASK_COMM_LEN instead.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Vladimir Zapolskiy <vzapolskiy@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/uapi/linux/cn_proc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/cn_proc.h b/include/uapi/linux/cn_proc.h
index db210625cee8..351d02786350 100644
--- a/include/uapi/linux/cn_proc.h
+++ b/include/uapi/linux/cn_proc.h
@@ -110,7 +110,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];
 		} comm;
 
 		struct coredump_proc_event {
-- 
2.18.2

Re: [PATCH v2 1/4] cn_proc.h: use TASK_COMM_LEN instread of 16 in struct proc_event

[Kees Cook]

On Thu, Oct 07, 2021 at 12:07:49PM +0000, Yafang Shao wrote:
> struct comm_proc_event was introduced in commit
> f786ecba4158 ("connector: add comm change event report to proc connector").
> It seems that there is no strong reason we must define the comm as a
> hardcode 16 bytes. So we can use TASK_COMM_LEN instead.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Vladimir Zapolskiy <vzapolskiy@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Petr Mladek <pmladek@suse.com>
> ---
>  include/uapi/linux/cn_proc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/cn_proc.h b/include/uapi/linux/cn_proc.h
> index db210625cee8..351d02786350 100644
> --- a/include/uapi/linux/cn_proc.h
> +++ b/include/uapi/linux/cn_proc.h
> @@ -110,7 +110,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];
>  		} comm;

Hrmm. This is UAPI -- 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). This will either need to stay truncated, or may
need a new interface with a variable-sized structure...

-Kees

>  
>  		struct coredump_proc_event {
> -- 
> 2.18.2
> 

-- 
Kees Cook

Re: [PATCH v2 1/4] cn_proc.h: use TASK_COMM_LEN instread of 16 in struct proc_event

[Kees Cook]

On Thu, Oct 07, 2021 at 07:51:35AM -0700, Kees Cook wrote:
> On Thu, Oct 07, 2021 at 12:07:49PM +0000, Yafang Shao wrote:
> > struct comm_proc_event was introduced in commit
> > f786ecba4158 ("connector: add comm change event report to proc connector").
> > It seems that there is no strong reason we must define the comm as a
> > hardcode 16 bytes. So we can use TASK_COMM_LEN instead.
> > 
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Vladimir Zapolskiy <vzapolskiy@gmail.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Petr Mladek <pmladek@suse.com>
> > ---
> >  include/uapi/linux/cn_proc.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/uapi/linux/cn_proc.h b/include/uapi/linux/cn_proc.h
> > index db210625cee8..351d02786350 100644
> > --- a/include/uapi/linux/cn_proc.h
> > +++ b/include/uapi/linux/cn_proc.h
> > @@ -110,7 +110,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];
> >  		} comm;
> 
> Hrmm. This is UAPI -- 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). This will either need to stay truncated, or may
> need a new interface with a variable-sized structure...

Specifically, this is needed for this series:


diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 646ad385e490..34bcba25c488 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -230,7 +230,9 @@ 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 */

-- 
Kees Cook

Re: [PATCH v2 1/4] cn_proc.h: use TASK_COMM_LEN instread of 16 in struct proc_event

[Yafang Shao]

On Thu, Oct 7, 2021 at 11:09 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Oct 07, 2021 at 07:51:35AM -0700, Kees Cook wrote:
> > On Thu, Oct 07, 2021 at 12:07:49PM +0000, Yafang Shao wrote:
> > > struct comm_proc_event was introduced in commit
> > > f786ecba4158 ("connector: add comm change event report to proc connector").
> > > It seems that there is no strong reason we must define the comm as a
> > > hardcode 16 bytes. So we can use TASK_COMM_LEN instead.
> > >
> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > Cc: Vladimir Zapolskiy <vzapolskiy@gmail.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > Cc: Petr Mladek <pmladek@suse.com>
> > > ---
> > >  include/uapi/linux/cn_proc.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/uapi/linux/cn_proc.h b/include/uapi/linux/cn_proc.h
> > > index db210625cee8..351d02786350 100644
> > > --- a/include/uapi/linux/cn_proc.h
> > > +++ b/include/uapi/linux/cn_proc.h
> > > @@ -110,7 +110,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];
> > >             } comm;
> >
> > Hrmm. This is UAPI -- 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). This will either need to stay truncated, or may
> > need a new interface with a variable-sized structure...
>
> Specifically, this is needed for this series:
>
>
> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
> index 646ad385e490..34bcba25c488 100644
> --- a/drivers/connector/cn_proc.c
> +++ b/drivers/connector/cn_proc.c
> @@ -230,7 +230,9 @@ 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 */
>

Thanks for the suggestion.  I will keep the UAPI code as-is and change
it as you suggested.

-- 
Thanks
Yafang

[PATCH v2 2/4] fs/exec: use strscpy instead of strlcpy in __set_task_comm

[Yafang Shao]

Fix a warning by checkpatch -
WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
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..de804c566200 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(tsk->comm, buf, sizeof(tsk->comm));
 	task_unlock(tsk);
 	perf_event_comm(tsk, exec);
 }
-- 
2.18.2

Re: [PATCH v2 2/4] fs/exec: use strscpy instead of strlcpy in __set_task_comm

[Kees Cook]

On Thu, Oct 07, 2021 at 12:07:50PM +0000, Yafang Shao wrote:
> Fix a warning by checkpatch -
> WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Petr Mladek <pmladek@suse.com>

Acked-by: Kees Cook <keescook@chromium.org>

> ---
>  fs/exec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index a098c133d8d7..de804c566200 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(tsk->comm, buf, sizeof(tsk->comm));
>  	task_unlock(tsk);
>  	perf_event_comm(tsk, exec);
>  }
> -- 
> 2.18.2
> 

-- 
Kees Cook

[PATCH v2 3/4] sched.h: extend task comm from 16 to 24 for CONFIG_BASE_FULL

[Yafang Shao]

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-
    nvidia-modeset/

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-
    ...

Besides the in-tree kthreads listed above, the out-of-tree kthreads may
also be truncated:

    rtase_work_queu
    nvidia-modeset/
    UVM global queu
    UVM deferred re
    ...

We should improve this problem fundamentally.

This patch extends the size of task comm to 24 bytes, which is the
same length with workqueue's, for the CONFIG_BASE_FULL case. And for the
CONFIG_BASE_SMALL case, the size of task comm is still kept as 16 bytes.

Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/sched.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 39039ce8ac4c..e0796068dee0 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -275,7 +275,11 @@ struct task_group;
 #define get_current_state()	READ_ONCE(current->__state)
 
 /* Task command name length: */
+#if CONFIG_BASE_SMALL
 #define TASK_COMM_LEN			16
+#else
+#define TASK_COMM_LEN			24
+#endif
 
 extern void scheduler_tick(void);
 
-- 
2.18.2

Re: [PATCH v2 3/4] sched.h: extend task comm from 16 to 24 for CONFIG_BASE_FULL

[Peter Zijlstra]

On Thu, Oct 07, 2021 at 12:07:51PM +0000, Yafang Shao wrote:
> Besides the in-tree kthreads listed above, the out-of-tree kthreads may
> also be truncated:
> 
>     rtase_work_queu
>     nvidia-modeset/
>     UVM global queu
>     UVM deferred re
>     ...
> 

Fundamentally we don't give a crap about out of tree stuff. And their
behaviour or not should be of absolutely no concern what so ever for any
patch ever.

Re: [PATCH v2 3/4] sched.h: extend task comm from 16 to 24 for CONFIG_BASE_FULL

[Yafang Shao]

On Thu, Oct 7, 2021 at 8:55 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Oct 07, 2021 at 12:07:51PM +0000, Yafang Shao wrote:
> > Besides the in-tree kthreads listed above, the out-of-tree kthreads may
> > also be truncated:
> >
> >     rtase_work_queu
> >     nvidia-modeset/
> >     UVM global queu
> >     UVM deferred re
> >     ...
> >
>
> Fundamentally we don't give a crap about out of tree stuff. And their
> behaviour or not should be of absolutely no concern what so ever for any
> patch ever.

Thanks for the explanation, I will update the commit log.

-- 
Thanks
Yafang

Re: [PATCH v2 3/4] sched.h: extend task comm from 16 to 24 for CONFIG_BASE_FULL

[Kees Cook]

On Thu, Oct 07, 2021 at 12:07:51PM +0000, Yafang Shao wrote:
> 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-
>     nvidia-modeset/
> 
> 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-
>     ...
> 
> Besides the in-tree kthreads listed above, the out-of-tree kthreads may
> also be truncated:
> 
>     rtase_work_queu
>     nvidia-modeset/
>     UVM global queu
>     UVM deferred re
>     ...
> 
> We should improve this problem fundamentally.
> 
> This patch extends the size of task comm to 24 bytes, which is the
> same length with workqueue's, for the CONFIG_BASE_FULL case. And for the
> CONFIG_BASE_SMALL case, the size of task comm is still kept as 16 bytes.
> 
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

This, as expected, adds 8 bytes to task_struct, which is reasonable. I
don't see any easy places to consolidate other members to make this a
"free" change, but I did just remove 64 bytes from task_struct[1], so
this is okay. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

[1] https://lore.kernel.org/lkml/20210924025450.4138503-1-keescook@chromium.org/

-- 
Kees Cook

Re: [PATCH v2 3/4] sched.h: extend task comm from 16 to 24 for CONFIG_BASE_FULL

[Yafang Shao]

On Thu, Oct 7, 2021 at 10:56 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Oct 07, 2021 at 12:07:51PM +0000, Yafang Shao wrote:
> > 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-
> >     nvidia-modeset/
> >
> > 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-
> >     ...
> >
> > Besides the in-tree kthreads listed above, the out-of-tree kthreads may
> > also be truncated:
> >
> >     rtase_work_queu
> >     nvidia-modeset/
> >     UVM global queu
> >     UVM deferred re
> >     ...
> >
> > We should improve this problem fundamentally.
> >
> > This patch extends the size of task comm to 24 bytes, which is the
> > same length with workqueue's, for the CONFIG_BASE_FULL case. And for the
> > CONFIG_BASE_SMALL case, the size of task comm is still kept as 16 bytes.
> >
> > Suggested-by: Petr Mladek <pmladek@suse.com>
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>
> This, as expected, adds 8 bytes to task_struct, which is reasonable. I
> don't see any easy places to consolidate other members to make this a
> "free" change, but I did just remove 64 bytes from task_struct[1], so
> this is okay. :)
>

Cool!

> Reviewed-by: Kees Cook <keescook@chromium.org>
>

Thanks for the review.

> [1] https://lore.kernel.org/lkml/20210924025450.4138503-1-keescook@chromium.org/
>

-- 
Thanks
Yafang

[PATCH v2 4/4] kernel/kthread: show a warning if kthread's comm is truncated

[Yafang Shao]

Show a warning if task comm is truncated. Below is the result
of my test case:

truncated kthread comm:I-am-a-kthread-with-lon, pid:14 by 6 characters

Suggtested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>
---
 kernel/kthread.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 5b37a8567168..46b924c92078 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -399,12 +399,17 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 	if (!IS_ERR(task)) {
 		static const struct sched_param param = { .sched_priority = 0 };
 		char name[TASK_COMM_LEN];
+		int len;
 
 		/*
 		 * task is already visible to other tasks, so updating
 		 * COMM must be protected.
 		 */
-		vsnprintf(name, sizeof(name), namefmt, args);
+		len = vsnprintf(name, sizeof(name), namefmt, args);
+		if (len >= TASK_COMM_LEN) {
+			pr_warn("truncated kthread comm:%s, pid:%d by %d characters\n",
+				name, task->pid, len - TASK_COMM_LEN + 1);
+		}
 		set_task_comm(task, name);
 		/*
 		 * root may have changed our (kthreadd's) priority or CPU mask.
-- 
2.18.2

Re: [PATCH v2 4/4] kernel/kthread: show a warning if kthread's comm is truncated

[Kees Cook]

On Thu, Oct 07, 2021 at 12:07:52PM +0000, Yafang Shao wrote:
> Show a warning if task comm is truncated. Below is the result
> of my test case:
> 
> truncated kthread comm:I-am-a-kthread-with-lon, pid:14 by 6 characters
> 
> Suggtested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

I like that we get a warning now. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v2 4/4] kernel/kthread: show a warning if kthread's comm is truncated

[Steven Rostedt]

On Thu,  7 Oct 2021 12:07:52 +0000
Yafang Shao <laoar.shao@gmail.com> wrote:

> -		vsnprintf(name, sizeof(name), namefmt, args);
> +		len = vsnprintf(name, sizeof(name), namefmt, args);
> +		if (len >= TASK_COMM_LEN) {
> +			pr_warn("truncated kthread comm:%s, pid:%d by %d characters\n",
> +				name, task->pid, len - TASK_COMM_LEN + 1);

Instead of saying how many characters it is truncated to, what about just
showing what it was truncated to?

			pr_warn("truncated kthread comm from:%s to:%.*s for pid:%d\n",
				name, TASK_COMM_LEN - 1, name, task->pid);

?

-- Steve

> +		}

Re: [PATCH v2 4/4] kernel/kthread: show a warning if kthread's comm is truncated

[Yafang Shao]

On Fri, Oct 8, 2021 at 1:41 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu,  7 Oct 2021 12:07:52 +0000
> Yafang Shao <laoar.shao@gmail.com> wrote:
>
> > -             vsnprintf(name, sizeof(name), namefmt, args);
> > +             len = vsnprintf(name, sizeof(name), namefmt, args);
> > +             if (len >= TASK_COMM_LEN) {
> > +                     pr_warn("truncated kthread comm:%s, pid:%d by %d characters\n",
> > +                             name, task->pid, len - TASK_COMM_LEN + 1);
>
> Instead of saying how many characters it is truncated to, what about just
> showing what it was truncated to?
>
>                         pr_warn("truncated kthread comm from:%s to:%.*s for pid:%d\n",
>                                 name, TASK_COMM_LEN - 1, name, task->pid);
>
> ?
>

The 'name' is the truncated one. So it will be printed like,
[    0.222126] truncated kthread comm from:rcu_tasks_kthre
to:rcu_tasks_kthre for pid:10

If we want to show the full name, we have to use the namefmt,  which
is not suggested to use by Petr.
See also https://lore.kernel.org/lkml/YVXVBXSZ1m4ScvbX@alley/

Or we can do it as follows,

-               char name[TASK_COMM_LEN];
+                /* To show the full name if it will be truncated. */
+               char name[TASK_COMM_LEN + 8];

Then the full name will be printed:
[    0.222587] truncated kthread comm from:rcu_tasks_kthread
to:rcu_tasks_kthre for pid:10

But that seems a little overkill ?

-- 
Thanks
Yafang