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

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

[Yafang Shao]

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.

When I was implementing a new per-cpu kthread cfs_migration, I found the
comm of it "cfs_migration/%u" is truncated due to the limitation of
TASK_COMM_LEN. For example, the comm of the percpu thread on CPU10~19 are
all with the same name "cfs_migration/1", which will confuse the user. This
issue is not critical, because we can get the corresponding CPU from the
task's Cpus_allowed. But for kthreads correspoinding to other hardware
devices, it is not easy to get the detailed device info from task comm,
for example,

    jbd2/nvme0n1p2-
    xfs-reclaim/sdf

We can also shorten the name to work around this problem, but I find
there are so many truncated kthreads:

    rcu_tasks_kthre
    rcu_tasks_rude_
    rcu_tasks_trace
    poll_mpt3sas0_s
    ext4-rsv-conver
    xfs-reclaim/sd{a, b, c, ...}
    xfs-blockgc/sd{a, b, c, ...}
    xfs-inodegc/sd{a, b, c, ...}
    audit_send_repl
    ecryptfs-kthrea
    vfio-irqfd-clea
    jbd2/nvme0n1p2-
    ...

We should improve this problem fundamentally.

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.

After this patchset, the truncated kthreads listed above will be
displayed as:

    rcu_tasks_kthread
    rcu_tasks_rude_kthread
    rcu_tasks_trace_kthread
    poll_mpt3sas0_statu
    ext4-rsv-conversion
    xfs-reclaim/sdf1
    xfs-blockgc/sdf1
    xfs-inodegc/sdf1
    audit_send_reply
    ecryptfs-kthread
    vfio-irqfd-cleanup
    jbd2/nvme0n1p2-8

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 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 (5):
  elfcore: use TASK_COMM_LEN instead of 16 in prpsinfo
  connector: use __get_task_comm in proc_comm_connector
  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

 drivers/connector/cn_proc.c    | 5 ++++-
 fs/exec.c                      | 2 +-
 include/linux/elfcore-compat.h | 2 +-
 include/linux/elfcore.h        | 4 ++--
 include/linux/sched.h          | 4 ++++
 kernel/kthread.c               | 7 ++++++-
 6 files changed, 18 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH v4 1/5] elfcore: use TASK_COMM_LEN instead of 16 in prpsinfo

[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 was moved from include/uapi/linux/elfcore.h into
include/linux/elfcore.h in commit
1e6b57d6421f ("unexport linux/elfcore.h")

As it is not UAPI code, we can replace 16 with TASK_COMM_LEN without
worrying about breaking userspace things.

struct elf_prpsinfo is used to dump the task information in userspace
coredump or kernel vmcore. So I verified what will happen to vmcore if
I extend the size of TASK_COMM_LEN to 24. The result is that the vmcore
still work fine as expected, for example:

crash> ps
   PID    PPID  CPU       TASK        ST  %MEM     VSZ    RSS  COMM
>     0      0   0  ffffffff8501a940  RU   0.0       0      0  [swapper/0]
>     0      0   1  ffff996e00f81f80  RU   0.0       0      0  [swapper/1]
>     0      0   2  ffff996e00f80000  RU   0.0       0      0  [swapper/2]
>     0      0   3  ffff996e00f85e80  RU   0.0       0      0  [swapper/3]
>     0      0   4  ffff996e00f83f00  RU   0.0       0      0  [swapper/4]
      0      0   5  ffff996e00f8de80  RU   0.0       0      0  [swapper/5]
>     0      0   6  ffff996e00f8bf00  RU   0.0       0      0  [swapper/6]
>     0      0   7  ffff996e00f89f80  RU   0.0       0      0  [swapper/7]
>     0      0   8  ffff996e00f88000  RU   0.0       0      0  [swapper/8]
>     0      0   9  ffff996e00f93f00  RU   0.0       0      0  [swapper/9]
>     0      0  10  ffff996e00f91f80  RU   0.0       0      0  [swapper/10]
>     0      0  11  ffff996e00f90000  RU   0.0       0      0  [swapper/11]
>     0      0  12  ffff996e00f95e80  RU   0.0       0      0  [swapper/12]
>     0      0  13  ffff996e00f98000  RU   0.0       0      0  [swapper/13]
>     0      0  14  ffff996e00f9de80  RU   0.0       0      0  [swapper/14]
>     0      0  15  ffff996e00f9bf00  RU   0.0       0      0  [swapper/15]

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Yafang Shao <laoar.shao@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>
---
 include/linux/elfcore-compat.h | 2 +-
 include/linux/elfcore.h        | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h
index e272c3d452ce..8a52a782161d 100644
--- a/include/linux/elfcore-compat.h
+++ b/include/linux/elfcore-compat.h
@@ -43,7 +43,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];
 	char				pr_psargs[ELF_PRARGSZ];
 };
 
diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h
index 2aaa15779d50..ff4e4e455160 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]; /* 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

Re: [PATCH v4 1/5] elfcore: use TASK_COMM_LEN instead of 16 in prpsinfo

[Steven Rostedt]

On Wed, 13 Oct 2021 10:23:42 +0000
Yafang Shao <laoar.shao@gmail.com> wrote:

> 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 was moved from include/uapi/linux/elfcore.h into
> include/linux/elfcore.h in commit
> 1e6b57d6421f ("unexport linux/elfcore.h")
> 
> As it is not UAPI code, we can replace 16 with TASK_COMM_LEN without
> worrying about breaking userspace things.
> 
> struct elf_prpsinfo is used to dump the task information in userspace
> coredump or kernel vmcore. So I verified what will happen to vmcore if
> I extend the size of TASK_COMM_LEN to 24. The result is that the vmcore
> still work fine as expected, for example:
> 
> crash> ps  
>    PID    PPID  CPU       TASK        ST  %MEM     VSZ    RSS  COMM
> >     0      0   0  ffffffff8501a940  RU   0.0       0      0  [swapper/0]
> >     0      0   1  ffff996e00f81f80  RU   0.0       0      0  [swapper/1]
> >     0      0   2  ffff996e00f80000  RU   0.0       0      0  [swapper/2]
> >     0      0   3  ffff996e00f85e80  RU   0.0       0      0  [swapper/3]
> >     0      0   4  ffff996e00f83f00  RU   0.0       0      0  [swapper/4]  
>       0      0   5  ffff996e00f8de80  RU   0.0       0      0  [swapper/5]
> >     0      0   6  ffff996e00f8bf00  RU   0.0       0      0  [swapper/6]
> >     0      0   7  ffff996e00f89f80  RU   0.0       0      0  [swapper/7]
> >     0      0   8  ffff996e00f88000  RU   0.0       0      0  [swapper/8]
> >     0      0   9  ffff996e00f93f00  RU   0.0       0      0  [swapper/9]
> >     0      0  10  ffff996e00f91f80  RU   0.0       0      0  [swapper/10]
> >     0      0  11  ffff996e00f90000  RU   0.0       0      0  [swapper/11]
> >     0      0  12  ffff996e00f95e80  RU   0.0       0      0  [swapper/12]
> >     0      0  13  ffff996e00f98000  RU   0.0       0      0  [swapper/13]
> >     0      0  14  ffff996e00f9de80  RU   0.0       0      0  [swapper/14]
> >     0      0  15  ffff996e00f9bf00  RU   0.0       0      0  [swapper/15]  
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Yafang Shao <laoar.shao@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>
> ---
>  include/linux/elfcore-compat.h | 2 +-
>  include/linux/elfcore.h        | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h
> index e272c3d452ce..8a52a782161d 100644
> --- a/include/linux/elfcore-compat.h
> +++ b/include/linux/elfcore-compat.h
> @@ -43,7 +43,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];
>  	char				pr_psargs[ELF_PRARGSZ];
>  };

Nice clean up, but should we add "#include <linux/sched.h>" to this header,
to make sure that it pulls in TASK_COMM_LEN define and not just hope it
gets pulled in beforehand by chance?

>  
> diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h
> index 2aaa15779d50..ff4e4e455160 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]; /* filename of executable */
> +	char	pr_psargs[ELF_PRARGSZ];	 /* initial part of arg list */

This header is fine, as it pulls in sched/task_stack.h which includes
sched.h.

-- Steve

>  };
>  
>  static inline void elf_core_copy_regs(elf_gregset_t *elfregs, struct pt_regs *regs)

Re: [PATCH v4 1/5] elfcore: use TASK_COMM_LEN instead of 16 in prpsinfo

[Yafang Shao]

On Wed, Oct 13, 2021 at 9:11 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 13 Oct 2021 10:23:42 +0000
> Yafang Shao <laoar.shao@gmail.com> wrote:
>
> > 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 was moved from include/uapi/linux/elfcore.h into
> > include/linux/elfcore.h in commit
> > 1e6b57d6421f ("unexport linux/elfcore.h")
> >
> > As it is not UAPI code, we can replace 16 with TASK_COMM_LEN without
> > worrying about breaking userspace things.
> >
> > struct elf_prpsinfo is used to dump the task information in userspace
> > coredump or kernel vmcore. So I verified what will happen to vmcore if
> > I extend the size of TASK_COMM_LEN to 24. The result is that the vmcore
> > still work fine as expected, for example:
> >
> > crash> ps
> >    PID    PPID  CPU       TASK        ST  %MEM     VSZ    RSS  COMM
> > >     0      0   0  ffffffff8501a940  RU   0.0       0      0  [swapper/0]
> > >     0      0   1  ffff996e00f81f80  RU   0.0       0      0  [swapper/1]
> > >     0      0   2  ffff996e00f80000  RU   0.0       0      0  [swapper/2]
> > >     0      0   3  ffff996e00f85e80  RU   0.0       0      0  [swapper/3]
> > >     0      0   4  ffff996e00f83f00  RU   0.0       0      0  [swapper/4]
> >       0      0   5  ffff996e00f8de80  RU   0.0       0      0  [swapper/5]
> > >     0      0   6  ffff996e00f8bf00  RU   0.0       0      0  [swapper/6]
> > >     0      0   7  ffff996e00f89f80  RU   0.0       0      0  [swapper/7]
> > >     0      0   8  ffff996e00f88000  RU   0.0       0      0  [swapper/8]
> > >     0      0   9  ffff996e00f93f00  RU   0.0       0      0  [swapper/9]
> > >     0      0  10  ffff996e00f91f80  RU   0.0       0      0  [swapper/10]
> > >     0      0  11  ffff996e00f90000  RU   0.0       0      0  [swapper/11]
> > >     0      0  12  ffff996e00f95e80  RU   0.0       0      0  [swapper/12]
> > >     0      0  13  ffff996e00f98000  RU   0.0       0      0  [swapper/13]
> > >     0      0  14  ffff996e00f9de80  RU   0.0       0      0  [swapper/14]
> > >     0      0  15  ffff996e00f9bf00  RU   0.0       0      0  [swapper/15]
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Yafang Shao <laoar.shao@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>
> > ---
> >  include/linux/elfcore-compat.h | 2 +-
> >  include/linux/elfcore.h        | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h
> > index e272c3d452ce..8a52a782161d 100644
> > --- a/include/linux/elfcore-compat.h
> > +++ b/include/linux/elfcore-compat.h
> > @@ -43,7 +43,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];
> >       char                            pr_psargs[ELF_PRARGSZ];
> >  };
>
> Nice clean up, but should we add "#include <linux/sched.h>" to this header,
> to make sure that it pulls in TASK_COMM_LEN define and not just hope it
> gets pulled in beforehand by chance?
>

Sure, I will add it.
Thanks for the suggestion.

> >
> > diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h
> > index 2aaa15779d50..ff4e4e455160 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]; /* filename of executable */
> > +     char    pr_psargs[ELF_PRARGSZ];  /* initial part of arg list */
>
> This header is fine, as it pulls in sched/task_stack.h which includes
> sched.h.
>
> -- Steve
>
> >  };
> >
> >  static inline void elf_core_copy_regs(elf_gregset_t *elfregs, struct pt_regs *regs)
>


-- 
Thanks
Yafang

[PATCH v4 2/5] connector: use __get_task_comm in proc_comm_connector

[Yafang Shao]

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

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Yafang Shao <laoar.shao@gmail.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 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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 */
-- 
2.17.1

Re: [PATCH v4 2/5] connector: use __get_task_comm in proc_comm_connector

[Steven Rostedt]

On Wed, 13 Oct 2021 10:23:43 +0000
Yafang Shao <laoar.shao@gmail.com> wrote:

> --- 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 */

__get_task_comm() uses strncpy() which my understanding is, does not add
the nul terminating byte when truncating. Which changes the functionality
here. As all task comms have a terminating byte, the old method would copy
that and include it. This won't add the terminating byte if the buffer is
smaller than the comm, and that might cause issues.

-- Steve

Re: [PATCH v4 2/5] connector: use __get_task_comm in proc_comm_connector

[Yafang Shao]

On Wed, Oct 13, 2021 at 10:19 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 13 Oct 2021 10:23:43 +0000
> Yafang Shao <laoar.shao@gmail.com> wrote:
>
> > --- 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 */
>
> __get_task_comm() uses strncpy() which my understanding is, does not add
> the nul terminating byte when truncating. Which changes the functionality
> here. As all task comms have a terminating byte, the old method would copy
> that and include it. This won't add the terminating byte if the buffer is
> smaller than the comm, and that might cause issues.
>

Right, that is a problem.
It seems that we should add a new helper get_task_comm_may_truncated().

-- 
Thanks
Yafang

Re: [PATCH v4 2/5] connector: use __get_task_comm in proc_comm_connector

[Steven Rostedt]

On Thu, 14 Oct 2021 09:48:09 +0800
Yafang Shao <laoar.shao@gmail.com> wrote:

> > __get_task_comm() uses strncpy() which my understanding is, does not add
> > the nul terminating byte when truncating. Which changes the functionality
> > here. As all task comms have a terminating byte, the old method would copy
> > that and include it. This won't add the terminating byte if the buffer is
> > smaller than the comm, and that might cause issues.
> >  
> 
> Right, that is a problem.
> It seems that we should add a new helper get_task_comm_may_truncated().

Or simply change __get_task_comm() to:

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 nul terminated */
	buf[buf_size - 1] = '\0';
	task_unlock(tsk);
	return buf;
}

But that should probably be a separate patch.

-- Steve

Re: [PATCH v4 2/5] connector: use __get_task_comm in proc_comm_connector

[Yafang Shao]

On Thu, Oct 14, 2021 at 10:24 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 14 Oct 2021 09:48:09 +0800
> Yafang Shao <laoar.shao@gmail.com> wrote:
>
> > > __get_task_comm() uses strncpy() which my understanding is, does not add
> > > the nul terminating byte when truncating. Which changes the functionality
> > > here. As all task comms have a terminating byte, the old method would copy
> > > that and include it. This won't add the terminating byte if the buffer is
> > > smaller than the comm, and that might cause issues.
> > >
> >
> > Right, that is a problem.
> > It seems that we should add a new helper get_task_comm_may_truncated().
>
> Or simply change __get_task_comm() to:
>
> 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 nul terminated */
>         buf[buf_size - 1] = '\0';
>         task_unlock(tsk);
>         return buf;
> }
>

That is better! Thanks for the suggestion.

> But that should probably be a separate patch.
>

Sure.

-- 
Thanks
Yafang

Re: [PATCH v4 2/5] connector: use __get_task_comm in proc_comm_connector

[Kees Cook]

On Wed, Oct 13, 2021 at 10:24:18PM -0400, Steven Rostedt wrote:
> On Thu, 14 Oct 2021 09:48:09 +0800
> Yafang Shao <laoar.shao@gmail.com> wrote:
> 
> > > __get_task_comm() uses strncpy() which my understanding is, does not add
> > > the nul terminating byte when truncating. Which changes the functionality
> > > here. As all task comms have a terminating byte, the old method would copy
> > > that and include it. This won't add the terminating byte if the buffer is
> > > smaller than the comm, and that might cause issues.
> > >  
> > 
> > Right, that is a problem.
> > It seems that we should add a new helper get_task_comm_may_truncated().
> 
> Or simply change __get_task_comm() to:
> 
> 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 nul terminated */
> 	buf[buf_size - 1] = '\0';
> 	task_unlock(tsk);
> 	return buf;
> }
> 
> But that should probably be a separate patch.

strscpy_pad() is the right thing here -- it'll retain the NUL-fill
properties of strncpy and terminate correctly.

The use of non-terminating issue with strncpy() wasn't a problem here
because get_task_comm() would always make sure task->comm was
terminated. (It uses strlcpy(), which I think needs to be changed to
strscpy_pad() too...)

-- 
Kees Cook

Re: [PATCH v4 2/5] connector: use __get_task_comm in proc_comm_connector

[Yafang Shao]

On Thu, Oct 14, 2021 at 12:50 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Oct 13, 2021 at 10:24:18PM -0400, Steven Rostedt wrote:
> > On Thu, 14 Oct 2021 09:48:09 +0800
> > Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > > > __get_task_comm() uses strncpy() which my understanding is, does not add
> > > > the nul terminating byte when truncating. Which changes the functionality
> > > > here. As all task comms have a terminating byte, the old method would copy
> > > > that and include it. This won't add the terminating byte if the buffer is
> > > > smaller than the comm, and that might cause issues.
> > > >
> > >
> > > Right, that is a problem.
> > > It seems that we should add a new helper get_task_comm_may_truncated().
> >
> > Or simply change __get_task_comm() to:
> >
> > 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 nul terminated */
> >       buf[buf_size - 1] = '\0';
> >       task_unlock(tsk);
> >       return buf;
> > }
> >
> > But that should probably be a separate patch.
>
> strscpy_pad() is the right thing here -- it'll retain the NUL-fill
> properties of strncpy and terminate correctly.
>

strscpy_pad() can also work, and seems more simple.

> The use of non-terminating issue with strncpy() wasn't a problem here
> because get_task_comm() would always make sure task->comm was
> terminated. (It uses strlcpy(), which I think needs to be changed to
> strscpy_pad() too...)
>
> --
> Kees Cook



-- 
Thanks
Yafang

Re: [PATCH v4 2/5] connector: use __get_task_comm in proc_comm_connector

[Steven Rostedt]

On Thu, 14 Oct 2021 17:26:16 +0800
Yafang Shao <laoar.shao@gmail.com> wrote:

> > > But that should probably be a separate patch.  
> >
> > strscpy_pad() is the right thing here -- it'll retain the NUL-fill
> > properties of strncpy and terminate correctly.
> >  
> 
> strscpy_pad() can also work, and seems more simple.

I'm fine either way. As long as the string that is updated is terminated.

-- Steve

[PATCH v4 3/5] 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>
Acked-by: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.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.17.1

[PATCH v4 4/5] 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-
    xfs-reclaim/sdf

We can also shorten the name to work around this problem, but I find
there are so many truncated kthreads:

    rcu_tasks_kthre
    rcu_tasks_rude_
    rcu_tasks_trace
    poll_mpt3sas0_s
    ext4-rsv-conver
    xfs-reclaim/sd{a, b, c, ...}
    xfs-blockgc/sd{a, b, c, ...}
    xfs-inodegc/sd{a, b, c, ...}
    audit_send_repl
    ecryptfs-kthrea
    vfio-irqfd-clea
    jbd2/nvme0n1p2-
    ...

We should improve this problem fundamentally.

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>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.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 c1a927ddec64..d3f947ef7d0a 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.17.1

[PATCH v4 5/5] 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

Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.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.17.1