The patch titled
Subject: get rid of __get_task_comm()
has been added to the -mm mm-nonmm-unstable branch. Its filename is
get-rid-of-__get_task_comm.patch
This patch will shortly appear at
https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/get-rid-of-__get_task_comm.patch
This patch will later appear in the mm-nonmm-unstable branch at
git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a
reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
The -mm tree is included into linux-next via the mm-everything
branch at git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
and is updated there every 2-3 working days
------------------------------------------------------
From: Yafang Shao <laoar.shao@xxxxxxxxx>
Subject: get rid of __get_task_comm()
Date: Mon, 7 Oct 2024 22:49:05 +0800
Patch series "Improve the copy of task comm", v8.
Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the
length of task comm. Changes in the task comm could result in a
destination string that is overflow. Therefore, we should explicitly
ensure the destination string is always NUL-terminated, regardless of the
task comm. This approach will facilitate future extensions to the task
comm.
As suggested by Linus [0], we can identify all relevant code with the
following git grep command:
git grep 'memcpy.*->comm\>'
git grep 'kstrdup.*->comm\>'
git grep 'strncpy.*->comm\>'
git grep 'strcpy.*->comm\>'
PATCH #2~#4: memcpy
PATCH #5~#6: kstrdup
PATCH #7: strcpy
Please note that strncpy() is not included in this series as it is being
tracked by another effort. [1]
This patch (of 7):
We want to eliminate the use of __get_task_comm() for the following
reasons:
- The task_lock() is unnecessary
Quoted from Linus [0]:
: Since user space can randomly change their names anyway, using locking
: was always wrong for readers (for writers it probably does make sense
: to have some lock - although practically speaking nobody cares there
: either, but at least for a writer some kind of race could have
: long-term mixed results
Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@xxxxxxxxx
Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@xxxxxxxxx
Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@xxxxxxxxxxxxxx [0]
Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@xxxxxxxxxxxxxx/
Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@xxxxxxxxxxxxxx/ [0]
Link: https://github.com/KSPP/linux/issues/90 [1]
Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
Cc: Christian Brauner <brauner@xxxxxxxxxx>
Cc: Jan Kara <jack@xxxxxxx>
Cc: Eric Biederman <ebiederm@xxxxxxxxxxxx>
Cc: Kees Cook <keescook@xxxxxxxxxxxx>
Cc: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>
Cc: Matus Jokay <matus.jokay@xxxxxxxx>
Cc: Alejandro Colomar <alx@xxxxxxxxxx>
Cc: "Serge E. Hallyn" <serge@xxxxxxxxxx>
Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
Cc: Justin Stitt <justinstitt@xxxxxxxxxx>
Cc: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
Cc: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
Cc: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
Cc: David Airlie <airlied@xxxxxxxxx>
Cc: Eric Paris <eparis@xxxxxxxxxx>
Cc: James Morris <jmorris@xxxxxxxxx>
Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
Cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
Cc: Maxime Ripard <mripard@xxxxxxxxxx>
Cc: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
Cc: Paul Moore <paul@xxxxxxxxxxxxxx>
Cc: Quentin Monnet <qmo@xxxxxxxxxx>
Cc: Simon Horman <horms@xxxxxxxxxx>
Cc: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---
fs/exec.c | 10 ----------
fs/proc/array.c | 2 +-
include/linux/sched.h | 28 ++++++++++++++++++++++------
kernel/kthread.c | 2 +-
4 files changed, 24 insertions(+), 18 deletions(-)
--- a/fs/exec.c~get-rid-of-__get_task_comm
+++ a/fs/exec.c
@@ -1189,16 +1189,6 @@ static int unshare_sighand(struct task_s
return 0;
}
-char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
-{
- task_lock(tsk);
- /* Always NUL terminated and zero-padded */
- strscpy_pad(buf, tsk->comm, buf_size);
- task_unlock(tsk);
- return buf;
-}
-EXPORT_SYMBOL_GPL(__get_task_comm);
-
/*
* These functions flushes out all traces of the currently running executable
* so that a new one can be started
--- a/fs/proc/array.c~get-rid-of-__get_task_comm
+++ a/fs/proc/array.c
@@ -109,7 +109,7 @@ void proc_task_name(struct seq_file *m,
else if (p->flags & PF_KTHREAD)
get_kthread_comm(tcomm, sizeof(tcomm), p);
else
- __get_task_comm(tcomm, sizeof(tcomm), p);
+ get_task_comm(tcomm, p);
if (escape)
seq_escape_str(m, tcomm, ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\");
--- a/include/linux/sched.h~get-rid-of-__get_task_comm
+++ a/include/linux/sched.h
@@ -1121,9 +1121,12 @@ struct task_struct {
/*
* executable name, excluding path.
*
- * - normally initialized setup_new_exec()
- * - access it with [gs]et_task_comm()
- * - lock it with task_lock()
+ * - normally initialized begin_new_exec()
+ * - set it with set_task_comm()
+ * - strscpy_pad() to ensure it is always NUL-terminated and
+ * zero-padded
+ * - task_lock() to ensure the operation is atomic and the name is
+ * fully updated.
*/
char comm[TASK_COMM_LEN];
@@ -1938,10 +1941,23 @@ static inline void set_task_comm(struct
__set_task_comm(tsk, from, false);
}
-extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
+/*
+ * - Why not use task_lock()?
+ * User space can randomly change their names anyway, so locking for readers
+ * doesn't make sense. For writers, locking is probably necessary, as a race
+ * condition could lead to long-term mixed results.
+ * The strscpy_pad() in __set_task_comm() can ensure that the task comm is
+ * always NUL-terminated and zero-padded. Therefore the race condition between
+ * reader and writer is not an issue.
+ *
+ * - BUILD_BUG_ON() can help prevent the buf from being truncated.
+ * Since the callers don't perform any return value checks, this safeguard is
+ * necessary.
+ */
#define get_task_comm(buf, tsk) ({ \
- BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN); \
- __get_task_comm(buf, sizeof(buf), tsk); \
+ BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN); \
+ strscpy_pad(buf, (tsk)->comm); \
+ buf; \
})
#ifdef CONFIG_SMP
--- a/kernel/kthread.c~get-rid-of-__get_task_comm
+++ a/kernel/kthread.c
@@ -101,7 +101,7 @@ void get_kthread_comm(char *buf, size_t
struct kthread *kthread = to_kthread(tsk);
if (!kthread || !kthread->full_name) {
- __get_task_comm(buf, buf_size, tsk);
+ strscpy(buf, tsk->comm, buf_size);
return;
}
_
Patches currently in -mm which might be from laoar.shao@xxxxxxxxx are
get-rid-of-__get_task_comm.patch
auditsc-replace-memcpy-with-strscpy.patch
security-replace-memcpy-with-get_task_comm.patch
bpftool-ensure-task-comm-is-always-nul-terminated.patch
mm-util-fix-possible-race-condition-in-kstrdup.patch
mm-util-deduplicate-code-in-kstrdupkstrndupkmemdup_nul.patch
drm-replace-strcpy-with-strscpy.patch
