Skip to content

Commit

Permalink
get rid of __get_task_comm()
Browse files Browse the repository at this point in the history
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~torvalds#6:   kstrdup
PATCH torvalds#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/[email protected]
Link: https://lkml.kernel.org/r/[email protected]
Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/
Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0]
Link: KSPP#90 [1]
Signed-off-by: Yafang Shao <[email protected]>
Suggested-by: Linus Torvalds <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Eric Biederman <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Matus Jokay <[email protected]>
Cc: Alejandro Colomar <[email protected]>
Cc: "Serge E. Hallyn" <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Justin Stitt <[email protected]>
Cc: Steven Rostedt (Google) <[email protected]>
Cc: Tetsuo Handa <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Eric Paris <[email protected]>
Cc: James Morris <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Ondrej Mosnacek <[email protected]>
Cc: Paul Moore <[email protected]>
Cc: Quentin Monnet <[email protected]>
Cc: Simon Horman <[email protected]>
Cc: Stephen Smalley <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
  • Loading branch information
laoar authored and akpm00 committed Nov 6, 2024
1 parent f9a4d89 commit 4cc0473
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 18 deletions.
10 changes: 0 additions & 10 deletions fs/exec.c
Original file line number Diff line number Diff line change
Expand Up @@ -1189,16 +1189,6 @@ static int unshare_sighand(struct task_struct *me)
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
Expand Down
2 changes: 1 addition & 1 deletion fs/proc/array.c
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
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\\");
Expand Down
28 changes: 22 additions & 6 deletions include/linux/sched.h
Original file line number Diff line number Diff line change
Expand Up @@ -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];

Expand Down Expand Up @@ -1938,10 +1941,23 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
__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
Expand Down
2 changes: 1 addition & 1 deletion kernel/kthread.c
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk)
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;
}

Expand Down

0 comments on commit 4cc0473

Please sign in to comment.