[PATCH 0/5] kthread: increase the size of kthread's comm

[PATCH 0/5] kthread: increase the size of kthread's comm

[Yafang Shao]

When I was implementing a new kthread cfs_migration [1], I found the
comm of it is trucated due to the limitation of TASK_COMM_LEN. After I
checked the other kthreads, I found some of them are also truncated, for
example,

    rcu_tasks_kthre
    rcu_tasks_rude_
    rcu_tasks_trace
    ecryptfs-kthrea
    vfio-irqfd-clea
    ext4-rsv-conver
    jbd2/nvme0n1p2-
    ...
    
Besides the in-tree kthreads listed above, the out-of-tree kthreads may
also be trucated, for example,
    
    rtase_work_queu
    nvidia-modeset/
    UVM global queu
    UVM deferred re
    ...
    
That motivates me to do this improvement.

This patch increases the size of ktread's comm from 16 to 24, which is
the same with workqueue's. After this change, the name of kthread can be
fully displayed in /proc/[pid]/comm, for example,
    
    rcu_tasks_kthread
    rcu_tasks_rude_kthread
    rcu_tasks_trace_kthread
    ecryptfs-kthread
    vfio-irqfd-cleanup
    ext4-rsv-conversion
    jbd2/nvme0n1p2-8
    ...
    
Because there're only a few of kthreads, so it won't increase too much
memory consumption.

After this improvement, if the comm of a kthread is still trucated, a
warning will be displayed. Below is the result of my test case -

__kthread_create_on_node:410: comm of pid 14 is truncated from "I-am-a-kthread-with-long-name" to "I-am-a-kthread-with-lon"

[1]. https://lore.kernel.org/lkml/YMmlAP%2FQhE6SWhCF@hirez.programming.kicks-ass.net/

Yafang Shao (5):
  kernel: replace sizeof(task->comm) with TASK_COMM_LEN
  kernel/fork: allocate task->comm dynamicly
  kernel/sched: improve the BUILD_BUG_ON() in get_task_comm()
  kernel: increase the size of kthread's comm
  kernel/kthread: show a warning if kthread's comm is still trucated

 arch/ia64/kernel/mca.c         |  6 +++---
 drivers/block/drbd/drbd_main.c |  2 +-
 drivers/hwtracing/stm/core.c   |  2 +-
 drivers/tty/tty_audit.c        |  2 +-
 fs/exec.c                      |  5 ++++-
 include/linux/sched.h          |  6 ++++--
 kernel/audit.c                 |  4 ++--
 kernel/auditsc.c               |  4 ++--
 kernel/capability.c            |  4 ++--
 kernel/fork.c                  | 26 ++++++++++++++++++++++++++
 kernel/futex.c                 |  2 +-
 kernel/kthread.c               |  8 ++++++--
 kernel/sys.c                   |  2 +-
 kernel/trace/blktrace.c        |  2 +-
 security/lsm_audit.c           |  4 ++--
 security/selinux/selinuxfs.c   |  2 +-
 security/yama/yama_lsm.c       |  2 +-
 sound/core/oss/pcm_oss.c       |  2 +-
 18 files changed, 60 insertions(+), 25 deletions(-)

-- 
2.17.1

[PATCH 1/5] kernel: replace sizeof(task->comm) with TASK_COMM_LEN

[Yafang Shao]

We can use TASK_COMM_LEN directly instread of sizeof(task->comm).

This patch also replace strlcpy with strscpy, to fix the 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>
---
 arch/ia64/kernel/mca.c         | 6 +++---
 drivers/block/drbd/drbd_main.c | 2 +-
 drivers/hwtracing/stm/core.c   | 2 +-
 drivers/tty/tty_audit.c        | 2 +-
 fs/exec.c                      | 2 +-
 kernel/audit.c                 | 4 ++--
 kernel/auditsc.c               | 4 ++--
 kernel/capability.c            | 4 ++--
 kernel/futex.c                 | 2 +-
 kernel/sys.c                   | 2 +-
 kernel/trace/blktrace.c        | 2 +-
 security/lsm_audit.c           | 4 ++--
 security/selinux/selinuxfs.c   | 2 +-
 security/yama/yama_lsm.c       | 2 +-
 sound/core/oss/pcm_oss.c       | 2 +-
 15 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/arch/ia64/kernel/mca.c b/arch/ia64/kernel/mca.c
index e628a88607bb..4ee86e91ff5e 100644
--- a/arch/ia64/kernel/mca.c
+++ b/arch/ia64/kernel/mca.c
@@ -875,7 +875,7 @@ copy_reg(const u64 *fr, u64 fnat, unsigned long *tr, unsigned long *tnat)
 static void
 ia64_mca_modify_comm(const struct task_struct *previous_current)
 {
-	char *p, comm[sizeof(current->comm)];
+	char *p, comm[TASK_COMM_LEN];
 	if (previous_current->pid)
 		snprintf(comm, sizeof(comm), "%s %d",
 			current->comm, previous_current->pid);
@@ -889,7 +889,7 @@ ia64_mca_modify_comm(const struct task_struct *previous_current)
 			current->comm, l, previous_current->comm,
 			task_thread_info(previous_current)->cpu);
 	}
-	memcpy(current->comm, comm, sizeof(current->comm));
+	memcpy(current->comm, comm, TASK_COMM_LEN);
 }
 
 static void
@@ -1794,7 +1794,7 @@ format_mca_init_stack(void *mca_data, unsigned long offset,
 	p->parent = p->real_parent = p->group_leader = p;
 	INIT_LIST_HEAD(&p->children);
 	INIT_LIST_HEAD(&p->sibling);
-	strncpy(p->comm, type, sizeof(p->comm)-1);
+	strncpy(p->comm, type, TASK_COMM_LEN-1);
 }
 
 /* Caller prevents this from being called after init */
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index 55234a558e98..a7cf062e76ab 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -306,7 +306,7 @@ static int drbd_thread_setup(void *arg)
 	unsigned long flags;
 	int retval;
 
-	snprintf(current->comm, sizeof(current->comm), "drbd_%c_%s",
+	snprintf(current->comm, TASK_COMM_LEN, "drbd_%c_%s",
 		 thi->name[0],
 		 resource->name);
 
diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 2712e699ba08..8ec0a34a9ba6 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -631,7 +631,7 @@ static ssize_t stm_char_write(struct file *file, const char __user *buf,
 	 * point, try to use the task name and "default" policy entries.
 	 */
 	if (!stmf->output.nr_chans) {
-		char comm[sizeof(current->comm)];
+		char comm[TASK_COMM_LEN];
 		char *ids[] = { comm, "default", NULL };
 
 		get_task_comm(comm, current);
diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
index ca7afd7b2716..b98b1aef5f6f 100644
--- a/drivers/tty/tty_audit.c
+++ b/drivers/tty/tty_audit.c
@@ -69,7 +69,7 @@ static void tty_audit_log(const char *description, dev_t dev,
 
 	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_TTY);
 	if (ab) {
-		char name[sizeof(current->comm)];
+		char name[TASK_COMM_LEN];
 
 		audit_log_format(ab, "%s pid=%u uid=%u auid=%u ses=%u major=%d"
 				 " minor=%d comm=", description, pid, uid,
diff --git a/fs/exec.c b/fs/exec.c
index a098c133d8d7..021c9dc727bc 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, TASK_COMM_LEN);
 	task_unlock(tsk);
 	perf_event_comm(tsk, exec);
 }
diff --git a/kernel/audit.c b/kernel/audit.c
index 121d37e700a6..1fbd036e77be 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1549,7 +1549,7 @@ static void audit_log_multicast(int group, const char *op, int err)
 {
 	const struct cred *cred;
 	struct tty_struct *tty;
-	char comm[sizeof(current->comm)];
+	char comm[TASK_COMM_LEN];
 	struct audit_buffer *ab;
 
 	if (!audit_enabled)
@@ -2192,7 +2192,7 @@ void audit_put_tty(struct tty_struct *tty)
 void audit_log_task_info(struct audit_buffer *ab)
 {
 	const struct cred *cred;
-	char comm[sizeof(current->comm)];
+	char comm[TASK_COMM_LEN];
 	struct tty_struct *tty;
 
 	if (!ab)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 8dd73a64f921..295376d7d926 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2595,7 +2595,7 @@ void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
 		       enum audit_nfcfgop op, gfp_t gfp)
 {
 	struct audit_buffer *ab;
-	char comm[sizeof(current->comm)];
+	char comm[TASK_COMM_LEN];
 
 	ab = audit_log_start(audit_context(), gfp, AUDIT_NETFILTER_CFG);
 	if (!ab)
@@ -2616,7 +2616,7 @@ static void audit_log_task(struct audit_buffer *ab)
 	kuid_t auid, uid;
 	kgid_t gid;
 	unsigned int sessionid;
-	char comm[sizeof(current->comm)];
+	char comm[TASK_COMM_LEN];
 
 	auid = audit_get_loginuid(current);
 	sessionid = audit_get_sessionid(current);
diff --git a/kernel/capability.c b/kernel/capability.c
index 46a361dde042..4033ee837f63 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -45,7 +45,7 @@ __setup("no_file_caps", file_caps_disable);
 
 static void warn_legacy_capability_use(void)
 {
-	char name[sizeof(current->comm)];
+	char name[TASK_COMM_LEN];
 
 	pr_info_once("warning: `%s' uses 32-bit capabilities (legacy support in use)\n",
 		     get_task_comm(name, current));
@@ -69,7 +69,7 @@ static void warn_legacy_capability_use(void)
 
 static void warn_deprecated_v2(void)
 {
-	char name[sizeof(current->comm)];
+	char name[TASK_COMM_LEN];
 
 	pr_info_once("warning: `%s' uses deprecated v2 capabilities in a way that may be insecure\n",
 		     get_task_comm(name, current));
diff --git a/kernel/futex.c b/kernel/futex.c
index c15ad276fd15..6cd08156611e 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1703,7 +1703,7 @@ static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
 
 	if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
 		if (oparg < 0 || oparg > 31) {
-			char comm[sizeof(current->comm)];
+			char comm[TASK_COMM_LEN];
 			/*
 			 * kill this print and return -EINVAL when userspace
 			 * is sane again
diff --git a/kernel/sys.c b/kernel/sys.c
index 8fdac0d90504..bea1120c5579 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2265,7 +2265,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		unsigned long, arg4, unsigned long, arg5)
 {
 	struct task_struct *me = current;
-	unsigned char comm[sizeof(me->comm)];
+	unsigned char comm[TASK_COMM_LEN];
 	long error;
 
 	error = security_task_prctl(option, arg2, arg3, arg4, arg5);
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index c221e4c3f625..009f77ebda5a 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -124,7 +124,7 @@ static void trace_note_tsk(struct task_struct *tsk)
 	spin_lock_irqsave(&running_trace_lock, flags);
 	list_for_each_entry(bt, &running_trace_list, running_list) {
 		trace_note(bt, tsk->pid, BLK_TN_PROCESS, tsk->comm,
-			   sizeof(tsk->comm), 0);
+			   TASK_COMM_LEN, 0);
 	}
 	spin_unlock_irqrestore(&running_trace_lock, flags);
 }
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 5a5016ef43b0..eec54cb48845 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -208,7 +208,7 @@ static inline void print_ipv4_addr(struct audit_buffer *ab, __be32 addr,
 static void dump_common_audit_data(struct audit_buffer *ab,
 				   struct common_audit_data *a)
 {
-	char comm[sizeof(current->comm)];
+	char comm[TASK_COMM_LEN];
 
 	/*
 	 * To keep stack sizes in check force programers to notice if they
@@ -310,7 +310,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 		if (tsk) {
 			pid_t pid = task_tgid_nr(tsk);
 			if (pid) {
-				char comm[sizeof(tsk->comm)];
+				char comm[TASK_COMM_LEN];
 				audit_log_format(ab, " opid=%d ocomm=", pid);
 				audit_log_untrustedstring(ab,
 				    memcpy(comm, tsk->comm, sizeof(comm)));
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index e4cd7cb856f3..e529db76502d 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -752,7 +752,7 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
 		goto out;
 
 	if (new_value) {
-		char comm[sizeof(current->comm)];
+		char comm[TASK_COMM_LEN];
 
 		memcpy(comm, current->comm, sizeof(comm));
 		pr_warn_once("SELinux: %s (%d) set checkreqprot to 1. This is deprecated and will be rejected in a future kernel release.\n",
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 06e226166aab..178d7a138e98 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -75,7 +75,7 @@ static void report_access(const char *access, struct task_struct *target,
 				struct task_struct *agent)
 {
 	struct access_report_info *info;
-	char agent_comm[sizeof(agent->comm)];
+	char agent_comm[TASK_COMM_LEN];
 
 	assert_spin_locked(&target->alloc_lock); /* for target->comm */
 
diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
index 82a818734a5f..d541530236e1 100644
--- a/sound/core/oss/pcm_oss.c
+++ b/sound/core/oss/pcm_oss.c
@@ -2460,7 +2460,7 @@ static int snd_task_name(struct task_struct *task, char *name, size_t size)
 
 	if (snd_BUG_ON(!task || !name || size < 2))
 		return -EINVAL;
-	for (idx = 0; idx < sizeof(task->comm) && idx + 1 < size; idx++)
+	for (idx = 0; idx < TASK_COMM_LEN && idx + 1 < size; idx++)
 		name[idx] = task->comm[idx];
 	name[idx] = '\0';
 	return 0;
-- 
2.17.1

[PATCH 2/5] kernel/fork: allocate task->comm dynamicly

[Yafang Shao]

task->comm is defined as an array embedded in struct task_struct before.
This patch changes it to a char pointer. It will be allocated in the fork
and freed when the task is freed.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/linux/sched.h |  2 +-
 kernel/fork.c         | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index e12b524426b0..b387b5943db4 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1051,7 +1051,7 @@ struct task_struct {
 	 * - access it with [gs]et_task_comm()
 	 * - lock it with task_lock()
 	 */
-	char				comm[TASK_COMM_LEN];
+	char				*comm;
 
 	struct nameidata		*nameidata;
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 38681ad44c76..227aec240501 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -721,6 +721,20 @@ static void mmdrop_async(struct mm_struct *mm)
 	}
 }
 
+static int task_comm_alloc(struct task_struct *p)
+{
+	p->comm = kzalloc(TASK_COMM_LEN, GFP_KERNEL);
+	if (!p->comm)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void task_comm_free(struct task_struct *p)
+{
+	kfree(p->comm);
+}
+
 static inline void free_signal_struct(struct signal_struct *sig)
 {
 	taskstats_tgid_free(sig);
@@ -753,6 +767,7 @@ void __put_task_struct(struct task_struct *tsk)
 	bpf_task_storage_free(tsk);
 	exit_creds(tsk);
 	delayacct_tsk_free(tsk);
+	task_comm_free(tsk);
 	put_signal_struct(tsk->signal);
 	sched_core_free(tsk);
 
@@ -2076,6 +2091,10 @@ static __latent_entropy struct task_struct *copy_process(
 	if (data_race(nr_threads >= max_threads))
 		goto bad_fork_cleanup_count;
 
+	retval = task_comm_alloc(p);
+	if (retval)
+		goto bad_fork_cleanup_count;
+
 	delayacct_tsk_init(p);	/* Must remain after dup_task_struct() */
 	p->flags &= ~(PF_SUPERPRIV | PF_WQ_WORKER | PF_IDLE | PF_NO_SETAFFINITY);
 	p->flags |= PF_FORKNOEXEC;
-- 
2.17.1

[PATCH 3/5] kernel/sched: improve the BUILD_BUG_ON() in get_task_comm()

[Yafang Shao]

What we really want to guarantee is that the buf size can't be less than
TASK_COMM_LEN. While the size be greater than TASK_COMM_LEN is
acceptable.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/linux/sched.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index b387b5943db4..959eaef248fc 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1931,7 +1931,7 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
 
 extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
 #define get_task_comm(buf, tsk) ({			\
-	BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);	\
+	BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN);	\
 	__get_task_comm(buf, sizeof(buf), tsk);		\
 })
 
-- 
2.17.1

[PATCH 4/5] kernel: increase the size of kthread's comm

[Yafang Shao]

Some of the kthreads' comm are trucated due to the limitation of
TASK_COMM_LEN, for example,

  rcu_tasks_kthre
  rcu_tasks_rude_
  rcu_tasks_trace
  ecryptfs-kthrea
  vfio-irqfd-clea
  ext4-rsv-conver
  jbd2/nvme0n1p2-
  ...

Besides the in-tree kthreads listed above, the out-of-tree kthreads may
also be trucated, for example,

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

That is not expected by the author of these kthreads.

This patch increases the size of ktread's comm from 16 to 24, which is
the same with workqueue's, to improve this situation. After this cahnge,
the name of kthread can be fully displayed in /proc/[pid]/comm,
for example,

  rcu_tasks_kthread
  rcu_tasks_rude_kthread
  rcu_tasks_trace_kthread
  ecryptfs-kthread
  vfio-irqfd-cleanup
  ext4-rsv-conversion
  jbd2/nvme0n1p2-8
  ...

Because there're only a few of kthreads, so it won't increase too much
memory.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 fs/exec.c             | 5 ++++-
 include/linux/sched.h | 2 ++
 kernel/fork.c         | 9 ++++++++-
 kernel/kthread.c      | 2 +-
 4 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 021c9dc727bc..4bf0501b7766 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1222,9 +1222,12 @@ EXPORT_SYMBOL_GPL(__get_task_comm);
 
 void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
 {
+	size_t size;
+
 	task_lock(tsk);
 	trace_task_rename(tsk, buf);
-	strscpy(tsk->comm, buf, TASK_COMM_LEN);
+	size = tsk->flags & PF_KTHREAD ? KTHREAD_COMM_LEN : TASK_COMM_LEN;
+	strscpy(tsk->comm, buf, size);
 	task_unlock(tsk);
 	perf_event_comm(tsk, exec);
 }
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 959eaef248fc..6b336eba4ff6 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -276,6 +276,8 @@ struct task_group;
 
 /* Task command name length: */
 #define TASK_COMM_LEN			16
+/* KTHREAD_COMM_LEN must be >= TASK_COMM_LEN */
+#define KTHREAD_COMM_LEN		24
 
 extern void scheduler_tick(void);
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 227aec240501..a2939353383d 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -723,7 +723,14 @@ static void mmdrop_async(struct mm_struct *mm)
 
 static int task_comm_alloc(struct task_struct *p)
 {
-	p->comm = kzalloc(TASK_COMM_LEN, GFP_KERNEL);
+	size_t size;
+
+	/*
+	 * PF_KTHREAD may be cleared in exec, but the allocated memory can
+	 * be safely freed.
+	 */
+	size = p->flags & PF_KTHREAD ? KTHREAD_COMM_LEN : TASK_COMM_LEN;
+	p->comm = kzalloc(size, GFP_KERNEL);
 	if (!p->comm)
 		return -ENOMEM;
 
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 5b37a8567168..6def951c605a 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -398,7 +398,7 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 	task = create->result;
 	if (!IS_ERR(task)) {
 		static const struct sched_param param = { .sched_priority = 0 };
-		char name[TASK_COMM_LEN];
+		char name[KTHREAD_COMM_LEN];
 
 		/*
 		 * task is already visible to other tasks, so updating
-- 
2.17.1

[PATCH 5/5] kernel/kthread: show a warning if kthread's comm is still trucated

[Yafang Shao]

Show a warning if the ktrhead's comm is still trucated. Below is the
result of my test case -

__kthread_create_on_node:410: comm of pid 14 is truncated from "I-am-a-kthread-with-long-name" to "I-am-a-kthread-with-lon"

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 kernel/kthread.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 6def951c605a..aa093f1f423a 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -404,7 +404,11 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
 		 * task is already visible to other tasks, so updating
 		 * COMM must be protected.
 		 */
-		vsnprintf(name, sizeof(name), namefmt, args);
+		if (vsnprintf(name, KTHREAD_COMM_LEN, namefmt, args) >=
+		    KTHREAD_COMM_LEN)
+			pr_warn("%s:%d: comm of pid %d is truncated from \"%s\" to \"%s\"\n",
+				__func__, __LINE__, task->pid, namefmt, name);
+
 		set_task_comm(task, name);
 		/*
 		 * root may have changed our (kthreadd's) priority or CPU mask.
-- 
2.17.1

Re: [PATCH 2/5] kernel/fork: allocate task->comm dynamicly

[Yafang Shao]

On Wed, Sep 29, 2021 at 7:51 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> task->comm is defined as an array embedded in struct task_struct before.
> This patch changes it to a char pointer. It will be allocated in the fork
> and freed when the task is freed.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/linux/sched.h |  2 +-
>  kernel/fork.c         | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index e12b524426b0..b387b5943db4 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1051,7 +1051,7 @@ struct task_struct {
>          * - access it with [gs]et_task_comm()
>          * - lock it with task_lock()
>          */
> -       char                            comm[TASK_COMM_LEN];
> +       char                            *comm;
>
>         struct nameidata                *nameidata;
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 38681ad44c76..227aec240501 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -721,6 +721,20 @@ static void mmdrop_async(struct mm_struct *mm)
>         }
>  }
>
> +static int task_comm_alloc(struct task_struct *p)
> +{
> +       p->comm = kzalloc(TASK_COMM_LEN, GFP_KERNEL);
> +       if (!p->comm)
> +               return -ENOMEM;
> +
> +       return 0;
> +}
> +
> +static void task_comm_free(struct task_struct *p)
> +{
> +       kfree(p->comm);
> +}
> +
>  static inline void free_signal_struct(struct signal_struct *sig)
>  {
>         taskstats_tgid_free(sig);
> @@ -753,6 +767,7 @@ void __put_task_struct(struct task_struct *tsk)
>         bpf_task_storage_free(tsk);
>         exit_creds(tsk);
>         delayacct_tsk_free(tsk);
> +       task_comm_free(tsk);
>         put_signal_struct(tsk->signal);
>         sched_core_free(tsk);
>
> @@ -2076,6 +2091,10 @@ static __latent_entropy struct task_struct *copy_process(
>         if (data_race(nr_threads >= max_threads))
>                 goto bad_fork_cleanup_count;
>
> +       retval = task_comm_alloc(p);
> +       if (retval)
> +               goto bad_fork_cleanup_count;
> +
>         delayacct_tsk_init(p);  /* Must remain after dup_task_struct() */
>         p->flags &= ~(PF_SUPERPRIV | PF_WQ_WORKER | PF_IDLE | PF_NO_SETAFFINITY);
>         p->flags |= PF_FORKNOEXEC;
> --
> 2.17.1
>

After sending the series, I find that I forget to move the
task_comm_alloc() to the end of fork().  Below is the updated one,

---
 include/linux/sched.h |  2 +-
 kernel/fork.c         | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index e12b524426b0..b387b5943db4 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1051,7 +1051,7 @@ struct task_struct {
         * - access it with [gs]et_task_comm()
         * - lock it with task_lock()
         */
-       char                            comm[TASK_COMM_LEN];
+       char                            *comm;

        struct nameidata                *nameidata;

diff --git a/kernel/fork.c b/kernel/fork.c
index 38681ad44c76..d1e0c38464ed 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -721,6 +721,20 @@ static void mmdrop_async(struct mm_struct *mm)
        }
 }

+static int task_comm_alloc(struct task_struct *p)
+{
+       p->comm = kzalloc(TASK_COMM_LEN, GFP_KERNEL);
+       if (!p->comm)
+               return -ENOMEM;
+
+       return 0;
+}
+
+static void task_comm_free(struct task_struct *p)
+{
+       kfree(p->comm);
+}
+
 static inline void free_signal_struct(struct signal_struct *sig)
 {
        taskstats_tgid_free(sig);
@@ -753,6 +767,7 @@ void __put_task_struct(struct task_struct *tsk)
        bpf_task_storage_free(tsk);
        exit_creds(tsk);
        delayacct_tsk_free(tsk);
+       task_comm_free(tsk);
        put_signal_struct(tsk->signal);
        sched_core_free(tsk);

@@ -2352,6 +2367,10 @@ static __latent_entropy struct task_struct *copy_process(
                goto bad_fork_cancel_cgroup;
        }

+       retval = task_comm_alloc(p);
+       if (retval)
+               goto bad_fork_cancel_cgroup;
+
        /* past the last point of failure */
        if (pidfile)
                fd_install(pidfd, pidfile);


-- 
Thanks
Yafang

Re: [PATCH 1/5] kernel: replace sizeof(task->comm) with TASK_COMM_LEN

[Kees Cook]

On Wed, Sep 29, 2021 at 11:50:32AM +0000, Yafang Shao wrote:
> We can use TASK_COMM_LEN directly instread of sizeof(task->comm).

This change makes sense if we accept performing allocation for
task->comm, but I don't think that's a good idea. I'll reply to those
patches...

> This patch also replace strlcpy with strscpy, to fix the warning by
> checkpatch -
> WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/

I like this part of the patch, though! :)

-Kees

> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  arch/ia64/kernel/mca.c         | 6 +++---
>  drivers/block/drbd/drbd_main.c | 2 +-
>  drivers/hwtracing/stm/core.c   | 2 +-
>  drivers/tty/tty_audit.c        | 2 +-
>  fs/exec.c                      | 2 +-
>  kernel/audit.c                 | 4 ++--
>  kernel/auditsc.c               | 4 ++--
>  kernel/capability.c            | 4 ++--
>  kernel/futex.c                 | 2 +-
>  kernel/sys.c                   | 2 +-
>  kernel/trace/blktrace.c        | 2 +-
>  security/lsm_audit.c           | 4 ++--
>  security/selinux/selinuxfs.c   | 2 +-
>  security/yama/yama_lsm.c       | 2 +-
>  sound/core/oss/pcm_oss.c       | 2 +-
>  15 files changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/ia64/kernel/mca.c b/arch/ia64/kernel/mca.c
> index e628a88607bb..4ee86e91ff5e 100644
> --- a/arch/ia64/kernel/mca.c
> +++ b/arch/ia64/kernel/mca.c
> @@ -875,7 +875,7 @@ copy_reg(const u64 *fr, u64 fnat, unsigned long *tr, unsigned long *tnat)
>  static void
>  ia64_mca_modify_comm(const struct task_struct *previous_current)
>  {
> -	char *p, comm[sizeof(current->comm)];
> +	char *p, comm[TASK_COMM_LEN];
>  	if (previous_current->pid)
>  		snprintf(comm, sizeof(comm), "%s %d",
>  			current->comm, previous_current->pid);
> @@ -889,7 +889,7 @@ ia64_mca_modify_comm(const struct task_struct *previous_current)
>  			current->comm, l, previous_current->comm,
>  			task_thread_info(previous_current)->cpu);
>  	}
> -	memcpy(current->comm, comm, sizeof(current->comm));
> +	memcpy(current->comm, comm, TASK_COMM_LEN);
>  }
>  
>  static void
> @@ -1794,7 +1794,7 @@ format_mca_init_stack(void *mca_data, unsigned long offset,
>  	p->parent = p->real_parent = p->group_leader = p;
>  	INIT_LIST_HEAD(&p->children);
>  	INIT_LIST_HEAD(&p->sibling);
> -	strncpy(p->comm, type, sizeof(p->comm)-1);
> +	strncpy(p->comm, type, TASK_COMM_LEN-1);
>  }
>  
>  /* Caller prevents this from being called after init */
> diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
> index 55234a558e98..a7cf062e76ab 100644
> --- a/drivers/block/drbd/drbd_main.c
> +++ b/drivers/block/drbd/drbd_main.c
> @@ -306,7 +306,7 @@ static int drbd_thread_setup(void *arg)
>  	unsigned long flags;
>  	int retval;
>  
> -	snprintf(current->comm, sizeof(current->comm), "drbd_%c_%s",
> +	snprintf(current->comm, TASK_COMM_LEN, "drbd_%c_%s",
>  		 thi->name[0],
>  		 resource->name);
>  
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index 2712e699ba08..8ec0a34a9ba6 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -631,7 +631,7 @@ static ssize_t stm_char_write(struct file *file, const char __user *buf,
>  	 * point, try to use the task name and "default" policy entries.
>  	 */
>  	if (!stmf->output.nr_chans) {
> -		char comm[sizeof(current->comm)];
> +		char comm[TASK_COMM_LEN];
>  		char *ids[] = { comm, "default", NULL };
>  
>  		get_task_comm(comm, current);
> diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
> index ca7afd7b2716..b98b1aef5f6f 100644
> --- a/drivers/tty/tty_audit.c
> +++ b/drivers/tty/tty_audit.c
> @@ -69,7 +69,7 @@ static void tty_audit_log(const char *description, dev_t dev,
>  
>  	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_TTY);
>  	if (ab) {
> -		char name[sizeof(current->comm)];
> +		char name[TASK_COMM_LEN];
>  
>  		audit_log_format(ab, "%s pid=%u uid=%u auid=%u ses=%u major=%d"
>  				 " minor=%d comm=", description, pid, uid,
> diff --git a/fs/exec.c b/fs/exec.c
> index a098c133d8d7..021c9dc727bc 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, TASK_COMM_LEN);
>  	task_unlock(tsk);
>  	perf_event_comm(tsk, exec);
>  }
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 121d37e700a6..1fbd036e77be 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1549,7 +1549,7 @@ static void audit_log_multicast(int group, const char *op, int err)
>  {
>  	const struct cred *cred;
>  	struct tty_struct *tty;
> -	char comm[sizeof(current->comm)];
> +	char comm[TASK_COMM_LEN];
>  	struct audit_buffer *ab;
>  
>  	if (!audit_enabled)
> @@ -2192,7 +2192,7 @@ void audit_put_tty(struct tty_struct *tty)
>  void audit_log_task_info(struct audit_buffer *ab)
>  {
>  	const struct cred *cred;
> -	char comm[sizeof(current->comm)];
> +	char comm[TASK_COMM_LEN];
>  	struct tty_struct *tty;
>  
>  	if (!ab)
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 8dd73a64f921..295376d7d926 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2595,7 +2595,7 @@ void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
>  		       enum audit_nfcfgop op, gfp_t gfp)
>  {
>  	struct audit_buffer *ab;
> -	char comm[sizeof(current->comm)];
> +	char comm[TASK_COMM_LEN];
>  
>  	ab = audit_log_start(audit_context(), gfp, AUDIT_NETFILTER_CFG);
>  	if (!ab)
> @@ -2616,7 +2616,7 @@ static void audit_log_task(struct audit_buffer *ab)
>  	kuid_t auid, uid;
>  	kgid_t gid;
>  	unsigned int sessionid;
> -	char comm[sizeof(current->comm)];
> +	char comm[TASK_COMM_LEN];
>  
>  	auid = audit_get_loginuid(current);
>  	sessionid = audit_get_sessionid(current);
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 46a361dde042..4033ee837f63 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -45,7 +45,7 @@ __setup("no_file_caps", file_caps_disable);
>  
>  static void warn_legacy_capability_use(void)
>  {
> -	char name[sizeof(current->comm)];
> +	char name[TASK_COMM_LEN];
>  
>  	pr_info_once("warning: `%s' uses 32-bit capabilities (legacy support in use)\n",
>  		     get_task_comm(name, current));
> @@ -69,7 +69,7 @@ static void warn_legacy_capability_use(void)
>  
>  static void warn_deprecated_v2(void)
>  {
> -	char name[sizeof(current->comm)];
> +	char name[TASK_COMM_LEN];
>  
>  	pr_info_once("warning: `%s' uses deprecated v2 capabilities in a way that may be insecure\n",
>  		     get_task_comm(name, current));
> diff --git a/kernel/futex.c b/kernel/futex.c
> index c15ad276fd15..6cd08156611e 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1703,7 +1703,7 @@ static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
>  
>  	if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
>  		if (oparg < 0 || oparg > 31) {
> -			char comm[sizeof(current->comm)];
> +			char comm[TASK_COMM_LEN];
>  			/*
>  			 * kill this print and return -EINVAL when userspace
>  			 * is sane again
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 8fdac0d90504..bea1120c5579 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2265,7 +2265,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  		unsigned long, arg4, unsigned long, arg5)
>  {
>  	struct task_struct *me = current;
> -	unsigned char comm[sizeof(me->comm)];
> +	unsigned char comm[TASK_COMM_LEN];
>  	long error;
>  
>  	error = security_task_prctl(option, arg2, arg3, arg4, arg5);
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index c221e4c3f625..009f77ebda5a 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -124,7 +124,7 @@ static void trace_note_tsk(struct task_struct *tsk)
>  	spin_lock_irqsave(&running_trace_lock, flags);
>  	list_for_each_entry(bt, &running_trace_list, running_list) {
>  		trace_note(bt, tsk->pid, BLK_TN_PROCESS, tsk->comm,
> -			   sizeof(tsk->comm), 0);
> +			   TASK_COMM_LEN, 0);
>  	}
>  	spin_unlock_irqrestore(&running_trace_lock, flags);
>  }
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index 5a5016ef43b0..eec54cb48845 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -208,7 +208,7 @@ static inline void print_ipv4_addr(struct audit_buffer *ab, __be32 addr,
>  static void dump_common_audit_data(struct audit_buffer *ab,
>  				   struct common_audit_data *a)
>  {
> -	char comm[sizeof(current->comm)];
> +	char comm[TASK_COMM_LEN];
>  
>  	/*
>  	 * To keep stack sizes in check force programers to notice if they
> @@ -310,7 +310,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>  		if (tsk) {
>  			pid_t pid = task_tgid_nr(tsk);
>  			if (pid) {
> -				char comm[sizeof(tsk->comm)];
> +				char comm[TASK_COMM_LEN];
>  				audit_log_format(ab, " opid=%d ocomm=", pid);
>  				audit_log_untrustedstring(ab,
>  				    memcpy(comm, tsk->comm, sizeof(comm)));
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index e4cd7cb856f3..e529db76502d 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -752,7 +752,7 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
>  		goto out;
>  
>  	if (new_value) {
> -		char comm[sizeof(current->comm)];
> +		char comm[TASK_COMM_LEN];
>  
>  		memcpy(comm, current->comm, sizeof(comm));
>  		pr_warn_once("SELinux: %s (%d) set checkreqprot to 1. This is deprecated and will be rejected in a future kernel release.\n",
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index 06e226166aab..178d7a138e98 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -75,7 +75,7 @@ static void report_access(const char *access, struct task_struct *target,
>  				struct task_struct *agent)
>  {
>  	struct access_report_info *info;
> -	char agent_comm[sizeof(agent->comm)];
> +	char agent_comm[TASK_COMM_LEN];
>  
>  	assert_spin_locked(&target->alloc_lock); /* for target->comm */
>  
> diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
> index 82a818734a5f..d541530236e1 100644
> --- a/sound/core/oss/pcm_oss.c
> +++ b/sound/core/oss/pcm_oss.c
> @@ -2460,7 +2460,7 @@ static int snd_task_name(struct task_struct *task, char *name, size_t size)
>  
>  	if (snd_BUG_ON(!task || !name || size < 2))
>  		return -EINVAL;
> -	for (idx = 0; idx < sizeof(task->comm) && idx + 1 < size; idx++)
> +	for (idx = 0; idx < TASK_COMM_LEN && idx + 1 < size; idx++)
>  		name[idx] = task->comm[idx];
>  	name[idx] = '\0';
>  	return 0;
> -- 
> 2.17.1
> 

-- 
Kees Cook

Re: [PATCH 2/5] kernel/fork: allocate task->comm dynamicly

[Kees Cook]

On Wed, Sep 29, 2021 at 11:50:33AM +0000, Yafang Shao wrote:
> task->comm is defined as an array embedded in struct task_struct before.
> This patch changes it to a char pointer. It will be allocated in the fork
> and freed when the task is freed.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/linux/sched.h |  2 +-
>  kernel/fork.c         | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index e12b524426b0..b387b5943db4 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1051,7 +1051,7 @@ struct task_struct {
>  	 * - access it with [gs]et_task_comm()
>  	 * - lock it with task_lock()
>  	 */
> -	char				comm[TASK_COMM_LEN];
> +	char				*comm;

This, I think, is basically a non-starter. It adds another kmalloc to
the fork path without a well-justified reason. TASK_COMM_LEN is small,
yes, but why is growing it valuable enough to slow things down?

(Or, can you prove that this does NOT slow things down? It seems like
it would.)

-Kees

>  
>  	struct nameidata		*nameidata;
>  
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 38681ad44c76..227aec240501 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -721,6 +721,20 @@ static void mmdrop_async(struct mm_struct *mm)
>  	}
>  }
>  
> +static int task_comm_alloc(struct task_struct *p)
> +{
> +	p->comm = kzalloc(TASK_COMM_LEN, GFP_KERNEL);
> +	if (!p->comm)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static void task_comm_free(struct task_struct *p)
> +{
> +	kfree(p->comm);
> +}
> +
>  static inline void free_signal_struct(struct signal_struct *sig)
>  {
>  	taskstats_tgid_free(sig);
> @@ -753,6 +767,7 @@ void __put_task_struct(struct task_struct *tsk)
>  	bpf_task_storage_free(tsk);
>  	exit_creds(tsk);
>  	delayacct_tsk_free(tsk);
> +	task_comm_free(tsk);
>  	put_signal_struct(tsk->signal);
>  	sched_core_free(tsk);
>  
> @@ -2076,6 +2091,10 @@ static __latent_entropy struct task_struct *copy_process(
>  	if (data_race(nr_threads >= max_threads))
>  		goto bad_fork_cleanup_count;
>  
> +	retval = task_comm_alloc(p);
> +	if (retval)
> +		goto bad_fork_cleanup_count;
> +
>  	delayacct_tsk_init(p);	/* Must remain after dup_task_struct() */
>  	p->flags &= ~(PF_SUPERPRIV | PF_WQ_WORKER | PF_IDLE | PF_NO_SETAFFINITY);
>  	p->flags |= PF_FORKNOEXEC;
> -- 
> 2.17.1
> 

-- 
Kees Cook

Re: [PATCH 3/5] kernel/sched: improve the BUILD_BUG_ON() in get_task_comm()

[Kees Cook]

On Wed, Sep 29, 2021 at 11:50:34AM +0000, Yafang Shao wrote:
> What we really want to guarantee is that the buf size can't be less than
> TASK_COMM_LEN. While the size be greater than TASK_COMM_LEN is
> acceptable.

This makes sense when task_struct::comm is a pointer. (But I think it's
not a good idea to do that.)

-Kees

> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/linux/sched.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index b387b5943db4..959eaef248fc 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1931,7 +1931,7 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
>  
>  extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
>  #define get_task_comm(buf, tsk) ({			\
> -	BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);	\
> +	BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN);	\
>  	__get_task_comm(buf, sizeof(buf), tsk);		\
>  })
>  
> -- 
> 2.17.1
> 

-- 
Kees Cook

Re: [PATCH 4/5] kernel: increase the size of kthread's comm

[Kees Cook]

On Wed, Sep 29, 2021 at 11:50:35AM +0000, Yafang Shao wrote:
> This patch increases the size of ktread's comm from 16 to 24, which is
> the same with workqueue's, to improve this situation. After this cahnge,
> [...]
> Because there're only a few of kthreads, so it won't increase too much
> memory.

Even without the performance impact changes, the math here doesn't hold
either, since using kmalloc means there are slabs being allocated to hold
the task "comm"s now (which comes with overhead), and every task added
a pointer to those 16 bytes (i.e. 8 more bytes on 64-bit systems). So
this change, even if there was 0 overhead in using slabs, would be
identical to having just raised TASK_COMM_LEN to 24. 8 byte pointer,
16 byte allocation == 24 bytes.

-Kees

-- 
Kees Cook

Re: [PATCH 5/5] kernel/kthread: show a warning if kthread's comm is still trucated

[Kees Cook]

On Wed, Sep 29, 2021 at 11:50:36AM +0000, Yafang Shao wrote:
> Show a warning if the ktrhead's comm is still trucated. Below is the
> result of my test case -
> 
> __kthread_create_on_node:410: comm of pid 14 is truncated from "I-am-a-kthread-with-long-name" to "I-am-a-kthread-with-lon"
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

I like this check as a reasonable way to make kthread authors aware of
the TASK_COMM_LEN limitation.

-Kees

> ---
>  kernel/kthread.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 6def951c605a..aa093f1f423a 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -404,7 +404,11 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
>  		 * task is already visible to other tasks, so updating
>  		 * COMM must be protected.
>  		 */
> -		vsnprintf(name, sizeof(name), namefmt, args);
> +		if (vsnprintf(name, KTHREAD_COMM_LEN, namefmt, args) >=
> +		    KTHREAD_COMM_LEN)
> +			pr_warn("%s:%d: comm of pid %d is truncated from \"%s\" to \"%s\"\n",
> +				__func__, __LINE__, task->pid, namefmt, name);
> +
>  		set_task_comm(task, name);
>  		/*
>  		 * root may have changed our (kthreadd's) priority or CPU mask.
> -- 
> 2.17.1
> 

-- 
Kees Cook

Re: [PATCH 1/5] kernel: replace sizeof(task->comm) with TASK_COMM_LEN

[Yafang Shao]

On Thu, Sep 30, 2021 at 2:09 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Sep 29, 2021 at 11:50:32AM +0000, Yafang Shao wrote:
> > We can use TASK_COMM_LEN directly instread of sizeof(task->comm).
>
> This change makes sense if we accept performing allocation for
> task->comm, but I don't think that's a good idea. I'll reply to those
> patches...
>

Thanks for the review.


> > This patch also replace strlcpy with strscpy, to fix the warning by
> > checkpatch -
> > WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
>
> I like this part of the patch, though! :)
>

I will make it a separate patch.

>
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  arch/ia64/kernel/mca.c         | 6 +++---
> >  drivers/block/drbd/drbd_main.c | 2 +-
> >  drivers/hwtracing/stm/core.c   | 2 +-
> >  drivers/tty/tty_audit.c        | 2 +-
> >  fs/exec.c                      | 2 +-
> >  kernel/audit.c                 | 4 ++--
> >  kernel/auditsc.c               | 4 ++--
> >  kernel/capability.c            | 4 ++--
> >  kernel/futex.c                 | 2 +-
> >  kernel/sys.c                   | 2 +-
> >  kernel/trace/blktrace.c        | 2 +-
> >  security/lsm_audit.c           | 4 ++--
> >  security/selinux/selinuxfs.c   | 2 +-
> >  security/yama/yama_lsm.c       | 2 +-
> >  sound/core/oss/pcm_oss.c       | 2 +-
> >  15 files changed, 21 insertions(+), 21 deletions(-)
> >
> > diff --git a/arch/ia64/kernel/mca.c b/arch/ia64/kernel/mca.c
> > index e628a88607bb..4ee86e91ff5e 100644
> > --- a/arch/ia64/kernel/mca.c
> > +++ b/arch/ia64/kernel/mca.c
> > @@ -875,7 +875,7 @@ copy_reg(const u64 *fr, u64 fnat, unsigned long *tr, unsigned long *tnat)
> >  static void
> >  ia64_mca_modify_comm(const struct task_struct *previous_current)
> >  {
> > -     char *p, comm[sizeof(current->comm)];
> > +     char *p, comm[TASK_COMM_LEN];
> >       if (previous_current->pid)
> >               snprintf(comm, sizeof(comm), "%s %d",
> >                       current->comm, previous_current->pid);
> > @@ -889,7 +889,7 @@ ia64_mca_modify_comm(const struct task_struct *previous_current)
> >                       current->comm, l, previous_current->comm,
> >                       task_thread_info(previous_current)->cpu);
> >       }
> > -     memcpy(current->comm, comm, sizeof(current->comm));
> > +     memcpy(current->comm, comm, TASK_COMM_LEN);
> >  }
> >
> >  static void
> > @@ -1794,7 +1794,7 @@ format_mca_init_stack(void *mca_data, unsigned long offset,
> >       p->parent = p->real_parent = p->group_leader = p;
> >       INIT_LIST_HEAD(&p->children);
> >       INIT_LIST_HEAD(&p->sibling);
> > -     strncpy(p->comm, type, sizeof(p->comm)-1);
> > +     strncpy(p->comm, type, TASK_COMM_LEN-1);
> >  }
> >
> >  /* Caller prevents this from being called after init */
> > diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
> > index 55234a558e98..a7cf062e76ab 100644
> > --- a/drivers/block/drbd/drbd_main.c
> > +++ b/drivers/block/drbd/drbd_main.c
> > @@ -306,7 +306,7 @@ static int drbd_thread_setup(void *arg)
> >       unsigned long flags;
> >       int retval;
> >
> > -     snprintf(current->comm, sizeof(current->comm), "drbd_%c_%s",
> > +     snprintf(current->comm, TASK_COMM_LEN, "drbd_%c_%s",
> >                thi->name[0],
> >                resource->name);
> >
> > diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> > index 2712e699ba08..8ec0a34a9ba6 100644
> > --- a/drivers/hwtracing/stm/core.c
> > +++ b/drivers/hwtracing/stm/core.c
> > @@ -631,7 +631,7 @@ static ssize_t stm_char_write(struct file *file, const char __user *buf,
> >        * point, try to use the task name and "default" policy entries.
> >        */
> >       if (!stmf->output.nr_chans) {
> > -             char comm[sizeof(current->comm)];
> > +             char comm[TASK_COMM_LEN];
> >               char *ids[] = { comm, "default", NULL };
> >
> >               get_task_comm(comm, current);
> > diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
> > index ca7afd7b2716..b98b1aef5f6f 100644
> > --- a/drivers/tty/tty_audit.c
> > +++ b/drivers/tty/tty_audit.c
> > @@ -69,7 +69,7 @@ static void tty_audit_log(const char *description, dev_t dev,
> >
> >       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_TTY);
> >       if (ab) {
> > -             char name[sizeof(current->comm)];
> > +             char name[TASK_COMM_LEN];
> >
> >               audit_log_format(ab, "%s pid=%u uid=%u auid=%u ses=%u major=%d"
> >                                " minor=%d comm=", description, pid, uid,
> > diff --git a/fs/exec.c b/fs/exec.c
> > index a098c133d8d7..021c9dc727bc 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, TASK_COMM_LEN);
> >       task_unlock(tsk);
> >       perf_event_comm(tsk, exec);
> >  }
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 121d37e700a6..1fbd036e77be 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1549,7 +1549,7 @@ static void audit_log_multicast(int group, const char *op, int err)
> >  {
> >       const struct cred *cred;
> >       struct tty_struct *tty;
> > -     char comm[sizeof(current->comm)];
> > +     char comm[TASK_COMM_LEN];
> >       struct audit_buffer *ab;
> >
> >       if (!audit_enabled)
> > @@ -2192,7 +2192,7 @@ void audit_put_tty(struct tty_struct *tty)
> >  void audit_log_task_info(struct audit_buffer *ab)
> >  {
> >       const struct cred *cred;
> > -     char comm[sizeof(current->comm)];
> > +     char comm[TASK_COMM_LEN];
> >       struct tty_struct *tty;
> >
> >       if (!ab)
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 8dd73a64f921..295376d7d926 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2595,7 +2595,7 @@ void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
> >                      enum audit_nfcfgop op, gfp_t gfp)
> >  {
> >       struct audit_buffer *ab;
> > -     char comm[sizeof(current->comm)];
> > +     char comm[TASK_COMM_LEN];
> >
> >       ab = audit_log_start(audit_context(), gfp, AUDIT_NETFILTER_CFG);
> >       if (!ab)
> > @@ -2616,7 +2616,7 @@ static void audit_log_task(struct audit_buffer *ab)
> >       kuid_t auid, uid;
> >       kgid_t gid;
> >       unsigned int sessionid;
> > -     char comm[sizeof(current->comm)];
> > +     char comm[TASK_COMM_LEN];
> >
> >       auid = audit_get_loginuid(current);
> >       sessionid = audit_get_sessionid(current);
> > diff --git a/kernel/capability.c b/kernel/capability.c
> > index 46a361dde042..4033ee837f63 100644
> > --- a/kernel/capability.c
> > +++ b/kernel/capability.c
> > @@ -45,7 +45,7 @@ __setup("no_file_caps", file_caps_disable);
> >
> >  static void warn_legacy_capability_use(void)
> >  {
> > -     char name[sizeof(current->comm)];
> > +     char name[TASK_COMM_LEN];
> >
> >       pr_info_once("warning: `%s' uses 32-bit capabilities (legacy support in use)\n",
> >                    get_task_comm(name, current));
> > @@ -69,7 +69,7 @@ static void warn_legacy_capability_use(void)
> >
> >  static void warn_deprecated_v2(void)
> >  {
> > -     char name[sizeof(current->comm)];
> > +     char name[TASK_COMM_LEN];
> >
> >       pr_info_once("warning: `%s' uses deprecated v2 capabilities in a way that may be insecure\n",
> >                    get_task_comm(name, current));
> > diff --git a/kernel/futex.c b/kernel/futex.c
> > index c15ad276fd15..6cd08156611e 100644
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -1703,7 +1703,7 @@ static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
> >
> >       if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
> >               if (oparg < 0 || oparg > 31) {
> > -                     char comm[sizeof(current->comm)];
> > +                     char comm[TASK_COMM_LEN];
> >                       /*
> >                        * kill this print and return -EINVAL when userspace
> >                        * is sane again
> > diff --git a/kernel/sys.c b/kernel/sys.c
> > index 8fdac0d90504..bea1120c5579 100644
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -2265,7 +2265,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> >               unsigned long, arg4, unsigned long, arg5)
> >  {
> >       struct task_struct *me = current;
> > -     unsigned char comm[sizeof(me->comm)];
> > +     unsigned char comm[TASK_COMM_LEN];
> >       long error;
> >
> >       error = security_task_prctl(option, arg2, arg3, arg4, arg5);
> > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > index c221e4c3f625..009f77ebda5a 100644
> > --- a/kernel/trace/blktrace.c
> > +++ b/kernel/trace/blktrace.c
> > @@ -124,7 +124,7 @@ static void trace_note_tsk(struct task_struct *tsk)
> >       spin_lock_irqsave(&running_trace_lock, flags);
> >       list_for_each_entry(bt, &running_trace_list, running_list) {
> >               trace_note(bt, tsk->pid, BLK_TN_PROCESS, tsk->comm,
> > -                        sizeof(tsk->comm), 0);
> > +                        TASK_COMM_LEN, 0);
> >       }
> >       spin_unlock_irqrestore(&running_trace_lock, flags);
> >  }
> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> > index 5a5016ef43b0..eec54cb48845 100644
> > --- a/security/lsm_audit.c
> > +++ b/security/lsm_audit.c
> > @@ -208,7 +208,7 @@ static inline void print_ipv4_addr(struct audit_buffer *ab, __be32 addr,
> >  static void dump_common_audit_data(struct audit_buffer *ab,
> >                                  struct common_audit_data *a)
> >  {
> > -     char comm[sizeof(current->comm)];
> > +     char comm[TASK_COMM_LEN];
> >
> >       /*
> >        * To keep stack sizes in check force programers to notice if they
> > @@ -310,7 +310,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> >               if (tsk) {
> >                       pid_t pid = task_tgid_nr(tsk);
> >                       if (pid) {
> > -                             char comm[sizeof(tsk->comm)];
> > +                             char comm[TASK_COMM_LEN];
> >                               audit_log_format(ab, " opid=%d ocomm=", pid);
> >                               audit_log_untrustedstring(ab,
> >                                   memcpy(comm, tsk->comm, sizeof(comm)));
> > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > index e4cd7cb856f3..e529db76502d 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -752,7 +752,7 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
> >               goto out;
> >
> >       if (new_value) {
> > -             char comm[sizeof(current->comm)];
> > +             char comm[TASK_COMM_LEN];
> >
> >               memcpy(comm, current->comm, sizeof(comm));
> >               pr_warn_once("SELinux: %s (%d) set checkreqprot to 1. This is deprecated and will be rejected in a future kernel release.\n",
> > diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> > index 06e226166aab..178d7a138e98 100644
> > --- a/security/yama/yama_lsm.c
> > +++ b/security/yama/yama_lsm.c
> > @@ -75,7 +75,7 @@ static void report_access(const char *access, struct task_struct *target,
> >                               struct task_struct *agent)
> >  {
> >       struct access_report_info *info;
> > -     char agent_comm[sizeof(agent->comm)];
> > +     char agent_comm[TASK_COMM_LEN];
> >
> >       assert_spin_locked(&target->alloc_lock); /* for target->comm */
> >
> > diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
> > index 82a818734a5f..d541530236e1 100644
> > --- a/sound/core/oss/pcm_oss.c
> > +++ b/sound/core/oss/pcm_oss.c
> > @@ -2460,7 +2460,7 @@ static int snd_task_name(struct task_struct *task, char *name, size_t size)
> >
> >       if (snd_BUG_ON(!task || !name || size < 2))
> >               return -EINVAL;
> > -     for (idx = 0; idx < sizeof(task->comm) && idx + 1 < size; idx++)
> > +     for (idx = 0; idx < TASK_COMM_LEN && idx + 1 < size; idx++)
> >               name[idx] = task->comm[idx];
> >       name[idx] = '\0';
> >       return 0;
> > --
> > 2.17.1
> >
>
> --
> Kees Cook



--
Thanks
Yafang

Re: [PATCH 2/5] kernel/fork: allocate task->comm dynamicly

[Yafang Shao]

On Thu, Sep 30, 2021 at 2:11 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Sep 29, 2021 at 11:50:33AM +0000, Yafang Shao wrote:
> > task->comm is defined as an array embedded in struct task_struct before.
> > This patch changes it to a char pointer. It will be allocated in the fork
> > and freed when the task is freed.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  include/linux/sched.h |  2 +-
> >  kernel/fork.c         | 19 +++++++++++++++++++
> >  2 files changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index e12b524426b0..b387b5943db4 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1051,7 +1051,7 @@ struct task_struct {
> >        * - access it with [gs]et_task_comm()
> >        * - lock it with task_lock()
> >        */
> > -     char                            comm[TASK_COMM_LEN];
> > +     char                            *comm;
>
> This, I think, is basically a non-starter. It adds another kmalloc to
> the fork path without a well-justified reason. TASK_COMM_LEN is small,
> yes, but why is growing it valuable enough to slow things down?
>
> (Or, can you prove that this does NOT slow things down? It seems like
> it would.)
>

Right, the new kmalloc would take some extra latency.
Seems it is not easy to measure which one is more valuable.

>
> >
> >       struct nameidata                *nameidata;
> >
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 38681ad44c76..227aec240501 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -721,6 +721,20 @@ static void mmdrop_async(struct mm_struct *mm)
> >       }
> >  }
> >
> > +static int task_comm_alloc(struct task_struct *p)
> > +{
> > +     p->comm = kzalloc(TASK_COMM_LEN, GFP_KERNEL);
> > +     if (!p->comm)
> > +             return -ENOMEM;
> > +
> > +     return 0;
> > +}
> > +
> > +static void task_comm_free(struct task_struct *p)
> > +{
> > +     kfree(p->comm);
> > +}
> > +
> >  static inline void free_signal_struct(struct signal_struct *sig)
> >  {
> >       taskstats_tgid_free(sig);
> > @@ -753,6 +767,7 @@ void __put_task_struct(struct task_struct *tsk)
> >       bpf_task_storage_free(tsk);
> >       exit_creds(tsk);
> >       delayacct_tsk_free(tsk);
> > +     task_comm_free(tsk);
> >       put_signal_struct(tsk->signal);
> >       sched_core_free(tsk);
> >
> > @@ -2076,6 +2091,10 @@ static __latent_entropy struct task_struct *copy_process(
> >       if (data_race(nr_threads >= max_threads))
> >               goto bad_fork_cleanup_count;
> >
> > +     retval = task_comm_alloc(p);
> > +     if (retval)
> > +             goto bad_fork_cleanup_count;
> > +
> >       delayacct_tsk_init(p);  /* Must remain after dup_task_struct() */
> >       p->flags &= ~(PF_SUPERPRIV | PF_WQ_WORKER | PF_IDLE | PF_NO_SETAFFINITY);
> >       p->flags |= PF_FORKNOEXEC;
> > --
> > 2.17.1
> >
>
> --
> Kees Cook



-- 
Thanks
Yafang

Re: [PATCH 3/5] kernel/sched: improve the BUILD_BUG_ON() in get_task_comm()

[Yafang Shao]

On Thu, Sep 30, 2021 at 2:12 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Sep 29, 2021 at 11:50:34AM +0000, Yafang Shao wrote:
> > What we really want to guarantee is that the buf size can't be less than
> > TASK_COMM_LEN. While the size be greater than TASK_COMM_LEN is
> > acceptable.
>
> This makes sense when task_struct::comm is a pointer. (But I think it's
> not a good idea to do that.)
>

OK, I won't make this change.

> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  include/linux/sched.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index b387b5943db4..959eaef248fc 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1931,7 +1931,7 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
> >
> >  extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
> >  #define get_task_comm(buf, tsk) ({                   \
> > -     BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);     \
> > +     BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN);      \
> >       __get_task_comm(buf, sizeof(buf), tsk);         \
> >  })
> >
> > --
> > 2.17.1
> >
>
> --
> Kees Cook



-- 
Thanks
Yafang

Re: [PATCH 4/5] kernel: increase the size of kthread's comm

[Yafang Shao]

On Thu, Sep 30, 2021 at 2:20 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Sep 29, 2021 at 11:50:35AM +0000, Yafang Shao wrote:
> > This patch increases the size of ktread's comm from 16 to 24, which is
> > the same with workqueue's, to improve this situation. After this cahnge,
> > [...]
> > Because there're only a few of kthreads, so it won't increase too much
> > memory.
>
> Even without the performance impact changes, the math here doesn't hold
> either, since using kmalloc means there are slabs being allocated to hold
> the task "comm"s now (which comes with overhead), and every task added
> a pointer to those 16 bytes (i.e. 8 more bytes on 64-bit systems). So
> this change, even if there was 0 overhead in using slabs, would be
> identical to having just raised TASK_COMM_LEN to 24. 8 byte pointer,
> 16 byte allocation == 24 bytes.
>

Right, thanks for the explanation. I missed the pointer before.

What about reusing the kthread_data() to store the the comm if the
kthread is not a kworker?

struct kthread {
     ...
     void *data;  // reuse this pointer
     ...
}

The logic will be something as follows,

    if (kthread_is_kworker) {
          store_worker_desc_into_kthread_data(); // already did in the kernel
     } else {
          store_comm_into_kthread_data();   // that is what we should change
    }

And then we modify the  proc_task_name() correspondingly.

-- 
Thanks
Yafang

Re: [PATCH 5/5] kernel/kthread: show a warning if kthread's comm is still trucated

[Yafang Shao]

On Thu, Sep 30, 2021 at 2:20 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Sep 29, 2021 at 11:50:36AM +0000, Yafang Shao wrote:
> > Show a warning if the ktrhead's comm is still trucated. Below is the
> > result of my test case -
> >
> > __kthread_create_on_node:410: comm of pid 14 is truncated from "I-am-a-kthread-with-long-name" to "I-am-a-kthread-with-lon"
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>
> I like this check as a reasonable way to make kthread authors aware of
> the TASK_COMM_LEN limitation.
>

Thanks for the review.

> > ---
> >  kernel/kthread.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > index 6def951c605a..aa093f1f423a 100644
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -404,7 +404,11 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
> >                * task is already visible to other tasks, so updating
> >                * COMM must be protected.
> >                */
> > -             vsnprintf(name, sizeof(name), namefmt, args);
> > +             if (vsnprintf(name, KTHREAD_COMM_LEN, namefmt, args) >=
> > +                 KTHREAD_COMM_LEN)
> > +                     pr_warn("%s:%d: comm of pid %d is truncated from \"%s\" to \"%s\"\n",
> > +                             __func__, __LINE__, task->pid, namefmt, name);
> > +
> >               set_task_comm(task, name);
> >               /*
> >                * root may have changed our (kthreadd's) priority or CPU mask.
> > --
> > 2.17.1
> >
>
> --
> Kees Cook



-- 
Thanks
Yafang

Re: [PATCH 2/5] kernel/fork: allocate task->comm dynamicly

[Petr Mladek]

On Thu 2021-09-30 20:41:40, Yafang Shao wrote:
> On Thu, Sep 30, 2021 at 2:11 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, Sep 29, 2021 at 11:50:33AM +0000, Yafang Shao wrote:
> > > task->comm is defined as an array embedded in struct task_struct before.
> > > This patch changes it to a char pointer. It will be allocated in the fork
> > > and freed when the task is freed.
> > >
> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > ---
> > >  include/linux/sched.h |  2 +-
> > >  kernel/fork.c         | 19 +++++++++++++++++++
> > >  2 files changed, 20 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > index e12b524426b0..b387b5943db4 100644
> > > --- a/include/linux/sched.h
> > > +++ b/include/linux/sched.h
> > > @@ -1051,7 +1051,7 @@ struct task_struct {
> > >        * - access it with [gs]et_task_comm()
> > >        * - lock it with task_lock()
> > >        */
> > > -     char                            comm[TASK_COMM_LEN];
> > > +     char                            *comm;
> >
> > This, I think, is basically a non-starter. It adds another kmalloc to
> > the fork path without a well-justified reason. TASK_COMM_LEN is small,
> > yes, but why is growing it valuable enough to slow things down?
> >
> > (Or, can you prove that this does NOT slow things down? It seems like
> > it would.)
> >
> 
> Right, the new kmalloc would take some extra latency.
> Seems it is not easy to measure which one is more valuable.

Honestly, I do not think that this exercise is worth it. The patchset
adds a lot of complexity and potential problems just to extend
comm from 16 to 24 for kthreads.

Is the problem real or just cosmetic?

If you really want it then it would be much easier to increase
TASK_COMM_LEN. task_struct is growing rather regularly. Extra
8 bytes should be acceptable.

If you want to make it more acceptable then keep 16 for
CONFIG_BASE_SMALL.


> > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > index 38681ad44c76..227aec240501 100644
> > > --- a/kernel/fork.c
> > > +++ b/kernel/fork.c
> > > @@ -753,6 +767,7 @@ void __put_task_struct(struct task_struct *tsk)
> > >       bpf_task_storage_free(tsk);
> > >       exit_creds(tsk);
> > >       delayacct_tsk_free(tsk);
> > > +     task_comm_free(tsk);

Just one example of the potential problems. Are you sure that nobody
will access tsk->comm after this point?

task->comm is widely used to describe the affected task_struct because
it is user friendly.

Also __put_task_struct() later calls also profile_handoff_task() that might
get registered even by some external module.

Best Regards,
Petr

PS: I think that the fork performance is important. It is tested by
benchmarks, for example, lmbench. But for me, the reliability is even
more important and any pointer/alloc/free just adds another weak
point.

Re: [PATCH 5/5] kernel/kthread: show a warning if kthread's comm is still trucated

[Petr Mladek]

On Wed 2021-09-29 11:50:36, Yafang Shao wrote:
> Show a warning if the ktrhead's comm is still trucated. Below is the
> result of my test case -
> 
> __kthread_create_on_node:410: comm of pid 14 is truncated from "I-am-a-kthread-with-long-name" to "I-am-a-kthread-with-lon"
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  kernel/kthread.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 6def951c605a..aa093f1f423a 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -404,7 +404,11 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
>  		 * task is already visible to other tasks, so updating
>  		 * COMM must be protected.
>  		 */
> -		vsnprintf(name, sizeof(name), namefmt, args);
> +		if (vsnprintf(name, KTHREAD_COMM_LEN, namefmt, args) >=
> +		    KTHREAD_COMM_LEN)
> +			pr_warn("%s:%d: comm of pid %d is truncated from \"%s\" to \"%s\"\n",
> +				__func__, __LINE__, task->pid, namefmt, name);

The warning makes sense. But the use of "namefmt" looks wrong. It is
format and not the name. Also __func__ and __LINE__ is overkill. It will
be always the same.

I would do something like:

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

Best Regards,
Petr

Re: [PATCH 2/5] kernel/fork: allocate task->comm dynamicly

[Yafang Shao]

On Thu, Sep 30, 2021 at 10:51 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Thu 2021-09-30 20:41:40, Yafang Shao wrote:
> > On Thu, Sep 30, 2021 at 2:11 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Wed, Sep 29, 2021 at 11:50:33AM +0000, Yafang Shao wrote:
> > > > task->comm is defined as an array embedded in struct task_struct before.
> > > > This patch changes it to a char pointer. It will be allocated in the fork
> > > > and freed when the task is freed.
> > > >
> > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > > ---
> > > >  include/linux/sched.h |  2 +-
> > > >  kernel/fork.c         | 19 +++++++++++++++++++
> > > >  2 files changed, 20 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > index e12b524426b0..b387b5943db4 100644
> > > > --- a/include/linux/sched.h
> > > > +++ b/include/linux/sched.h
> > > > @@ -1051,7 +1051,7 @@ struct task_struct {
> > > >        * - access it with [gs]et_task_comm()
> > > >        * - lock it with task_lock()
> > > >        */
> > > > -     char                            comm[TASK_COMM_LEN];
> > > > +     char                            *comm;
> > >
> > > This, I think, is basically a non-starter. It adds another kmalloc to
> > > the fork path without a well-justified reason. TASK_COMM_LEN is small,
> > > yes, but why is growing it valuable enough to slow things down?
> > >
> > > (Or, can you prove that this does NOT slow things down? It seems like
> > > it would.)
> > >
> >
> > Right, the new kmalloc would take some extra latency.
> > Seems it is not easy to measure which one is more valuable.
>
> Honestly, I do not think that this exercise is worth it. The patchset
> adds a lot of complexity and potential problems just to extend
> comm from 16 to 24 for kthreads.
>
> Is the problem real or just cosmetic?
>

It is a problem, but not a critical problem.

Take the "cfs_migration/%u" for example.
It will be truncated to "cfs_migration/1" for CPU 10~19, which will
make the user confused.  But as it is a per-cpu thread, the user can
get its CPU information from its cpu mask.  And we can also shorten
its name to work around this issue.

But for kthreads corresponding to some other hardware devices, it may
not be easy to get the detailed information from the task's comm. For
example,
    jbd2/nvme0n1p2-
    nvidia-modeset/


> If you really want it then it would be much easier to increase
> TASK_COMM_LEN. task_struct is growing rather regularly. Extra
> 8 bytes should be acceptable.
>
> If you want to make it more acceptable then keep 16 for
> CONFIG_BASE_SMALL.
>

That seems to be a possible solution.

>
> > > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > > index 38681ad44c76..227aec240501 100644
> > > > --- a/kernel/fork.c
> > > > +++ b/kernel/fork.c
> > > > @@ -753,6 +767,7 @@ void __put_task_struct(struct task_struct *tsk)
> > > >       bpf_task_storage_free(tsk);
> > > >       exit_creds(tsk);
> > > >       delayacct_tsk_free(tsk);
> > > > +     task_comm_free(tsk);
>
> Just one example of the potential problems. Are you sure that nobody
> will access tsk->comm after this point?
>

That is a risk.
Should free it in free_task(), just before free_task_struct().

> task->comm is widely used to describe the affected task_struct because
> it is user friendly.
>
> Also __put_task_struct() later calls also profile_handoff_task() that might
> get registered even by some external module.
>
> Best Regards,
> Petr
>
> PS: I think that the fork performance is important. It is tested by
> benchmarks, for example, lmbench. But for me, the reliability is even
> more important and any pointer/alloc/free just adds another weak
> point.

Many thanks for the explanation.

-- 
Thanks
Yafang

Re: [PATCH 5/5] kernel/kthread: show a warning if kthread's comm is still trucated

[Yafang Shao]

On Thu, Sep 30, 2021 at 11:17 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2021-09-29 11:50:36, Yafang Shao wrote:
> > Show a warning if the ktrhead's comm is still trucated. Below is the
> > result of my test case -
> >
> > __kthread_create_on_node:410: comm of pid 14 is truncated from "I-am-a-kthread-with-long-name" to "I-am-a-kthread-with-lon"
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  kernel/kthread.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > index 6def951c605a..aa093f1f423a 100644
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -404,7 +404,11 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
> >                * task is already visible to other tasks, so updating
> >                * COMM must be protected.
> >                */
> > -             vsnprintf(name, sizeof(name), namefmt, args);
> > +             if (vsnprintf(name, KTHREAD_COMM_LEN, namefmt, args) >=
> > +                 KTHREAD_COMM_LEN)
> > +                     pr_warn("%s:%d: comm of pid %d is truncated from \"%s\" to \"%s\"\n",
> > +                             __func__, __LINE__, task->pid, namefmt, name);
>
> The warning makes sense. But the use of "namefmt" looks wrong. It is
> format and not the name. Also __func__ and __LINE__ is overkill. It will
> be always the same.
>
> I would do something like:
>
>                 len = vsnprintf(name, sizeof(name), namefmt, args);
>                 if (len >= KTHREAD_COMM_LEN) {
>                         pr_warn("truncated kthread comm:%s, pid:%d by %d characters\n",
>                                 name, task->pid, len - KTHREAD_COMM_LEN + 1);
>                 }
>

Thanks for the suggestion. I will change it.

-- 
Thanks
Yafang

Re: [PATCH 1/5] kernel: replace sizeof(task->comm) with TASK_COMM_LEN

[Al Viro]

On Wed, Sep 29, 2021 at 11:50:32AM +0000, Yafang Shao wrote:

> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2265,7 +2265,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  		unsigned long, arg4, unsigned long, arg5)
>  {
>  	struct task_struct *me = current;
> -	unsigned char comm[sizeof(me->comm)];
> +	unsigned char comm[TASK_COMM_LEN];
>  	long error;
>  
>  	error = security_task_prctl(option, arg2, arg3, arg4, arg5);

Slightly below you have this:
        case PR_SET_NAME:
                comm[sizeof(me->comm) - 1] = 0;
                if (strncpy_from_user(comm, (char __user *)arg2,
                                      sizeof(me->comm) - 1) < 0)
                        return -EFAULT;
                set_task_comm(me, comm);
                proc_comm_connector(me);
                break;

How had that been tested?

Re: [PATCH 0/5] kthread: increase the size of kthread's comm

[Al Viro]

On Wed, Sep 29, 2021 at 11:50:31AM +0000, Yafang Shao wrote:

> That motivates me to do this improvement.
> 
> This patch increases the size of ktread's comm from 16 to 24, which is
> the same with workqueue's. After this change, the name of kthread can be
> fully displayed in /proc/[pid]/comm, for example,
>     
>     rcu_tasks_kthread
>     rcu_tasks_rude_kthread
>     rcu_tasks_trace_kthread
>     ecryptfs-kthread
>     vfio-irqfd-cleanup
>     ext4-rsv-conversion
>     jbd2/nvme0n1p2-8
>     ...
>     
> Because there're only a few of kthreads, so it won't increase too much
> memory consumption.

That's a bloody massive overkill.  If you care about /proc/*/comm, you
might want to take a look at the function that generates its contents:

void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
{
        char tcomm[64];

        if (p->flags & PF_WQ_WORKER)
                wq_worker_comm(tcomm, sizeof(tcomm), p);
        else   
                __get_task_comm(tcomm, sizeof(tcomm), p);

        if (escape)
                seq_escape_str(m, tcomm, ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\");
        else   
                seq_printf(m, "%.64s", tcomm);
}

Hint: it's not always p->comm verbatim...

Re: [PATCH 1/5] kernel: replace sizeof(task->comm) with TASK_COMM_LEN

[Yafang Shao]

On Sun, Oct 3, 2021 at 11:31 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Sep 29, 2021 at 11:50:32AM +0000, Yafang Shao wrote:
>
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -2265,7 +2265,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> >               unsigned long, arg4, unsigned long, arg5)
> >  {
> >       struct task_struct *me = current;
> > -     unsigned char comm[sizeof(me->comm)];
> > +     unsigned char comm[TASK_COMM_LEN];
> >       long error;
> >
> >       error = security_task_prctl(option, arg2, arg3, arg4, arg5);
>
> Slightly below you have this:
>         case PR_SET_NAME:
>                 comm[sizeof(me->comm) - 1] = 0;
>                 if (strncpy_from_user(comm, (char __user *)arg2,
>                                       sizeof(me->comm) - 1) < 0)
>                         return -EFAULT;
>                 set_task_comm(me, comm);
>                 proc_comm_connector(me);
>                 break;
>
> How had that been tested?

Thanks for pointing out this. Honestly speaking I didn't test  PR_SET_NAME.
It is strange that I missed these two me->comm.
I have grepped all "->comm"  in the kernel and checked one by one, so
these two should be found...

-- 
Thanks
Yafang

Re: [PATCH 0/5] kthread: increase the size of kthread's comm

[Yafang Shao]

On Sun, Oct 3, 2021 at 11:41 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Sep 29, 2021 at 11:50:31AM +0000, Yafang Shao wrote:
>
> > That motivates me to do this improvement.
> >
> > This patch increases the size of ktread's comm from 16 to 24, which is
> > the same with workqueue's. After this change, the name of kthread can be
> > fully displayed in /proc/[pid]/comm, for example,
> >
> >     rcu_tasks_kthread
> >     rcu_tasks_rude_kthread
> >     rcu_tasks_trace_kthread
> >     ecryptfs-kthread
> >     vfio-irqfd-cleanup
> >     ext4-rsv-conversion
> >     jbd2/nvme0n1p2-8
> >     ...
> >
> > Because there're only a few of kthreads, so it won't increase too much
> > memory consumption.
>
> That's a bloody massive overkill.  If you care about /proc/*/comm, you
> might want to take a look at the function that generates its contents:
>
> void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
> {
>         char tcomm[64];
>
>         if (p->flags & PF_WQ_WORKER)
>                 wq_worker_comm(tcomm, sizeof(tcomm), p);
>         else
>                 __get_task_comm(tcomm, sizeof(tcomm), p);
>
>         if (escape)
>                 seq_escape_str(m, tcomm, ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\");
>         else
>                 seq_printf(m, "%.64s", tcomm);
> }
>
> Hint: it's not always p->comm verbatim...

Right, I found it.  That's why I was wondering  storing the kthread's
comm in kthread_data.[1]
But after Petr's suggestion, I find increasing the size of comm to 24
for COFNIG_BASE_FULL and keeping it as 16 for COFNIG_BASE_SMALL is a
better solution.

[1]. https://lore.kernel.org/lkml/CALOAHbD3HUqUnjMYKX7NGwVWiS4K7OvS6uPNWucnOA5Cy3pn9w@mail.gmail.com/#t

-- 
Thanks
Yafang