[PATCH v3 01/11] fs/exec: Drop task_lock() inside __get_task_comm()
From: Yafang Shao <hidden>
Date: 2024-06-21 02:30:46
Also in:
bpf, dri-devel, linux-fsdevel, linux-mm, linux-security-module, linux-trace-kernel, selinux
Subsystem:
exec & binfmt api, elf, filesystems (vfs and infrastructure), scheduler, the rest · Maintainers:
Kees Cook, Alexander Viro, Christian Brauner, Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot, Linus Torvalds
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 Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com (local) [0] Signed-off-by: Yafang Shao <redacted> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <redacted> Cc: Kees Cook <redacted> Cc: Alexei Starovoitov <redacted> Cc: Matus Jokay <redacted> --- fs/exec.c | 10 ++++++++-- include/linux/sched.h | 4 ++-- 2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 40073142288f..fa6b61c79df8 100644
--- a/fs/exec.c
+++ b/fs/exec.c@@ -1238,12 +1238,18 @@ static int unshare_sighand(struct task_struct *me) return 0; } +/* + * 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. Therefore the race condition between reader and writer + * is not an issue. + */ 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);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 61591ac6eab6..95888d1da49e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h@@ -1086,9 +1086,9 @@ struct task_struct { /* * executable name, excluding path. * - * - normally initialized setup_new_exec() + * - normally initialized begin_new_exec() * - access it with [gs]et_task_comm() - * - lock it with task_lock() + * - lock it with task_lock() for writing */ char comm[TASK_COMM_LEN];
--
2.39.1