(2011/05/19 4:58), Linus Torvalds wrote:
On Tue, May 17, 2011 at 6:41 PM, John Stultz<john.stultz@xxxxxxxxxx> wrote:
While this was brought up at the time, it was not considered
problematic, as the comm writing was done in such a way that
only null or incomplete comms could be read. However, recently
folks have made it clear they want to see this issue resolved.
What folks?
I don't think a new lock (or any lock) is at all appropriate.
There's just no point. Just guarantee that the last byte is always
zero, and you're done.
If you just guarantee that, THERE IS NO RACE. The last byte never
changes. You may get odd half-way strings, but you've trivially
guaranteed that they are C NUL-terminated, with no locking, no memory
ordering, no nothing.
Anybody who asks for any locking is just being a silly git. Tell them
to man the f*ck up.
So I'm not going to apply anything like this for 2.6.39, but I'm also
not going to apply it for 40 or 41 or anything else.
I refuse to accept just stupid unnecessary crap.
Do every body agree this conclusion? If so, I'd like to propose
documentation update patch. Because I recently observed Dave Hansen
and David Rientjes discussed task->comm locking rule. So, I guess
current code comments is misleading. It doesn't describe why almost
all task->comm user don't take task_lock() at all.
What do you think?
From e96571a8d470156d6ab7f3656d938aab126f17e8 Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>
Date: Fri, 20 May 2011 19:26:12 +0900
Subject: [PATCH] add comments for task->comm locking rule
Now, sched.h says, we should use [gs]et_task_comm for task->comm
access. but almost all actual code don't take task_lock(). It
brought repeated almost same locking rule discussion. Probably
we have to write exact current locking rule explicitly.
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>
Cc: John Stultz <john.stultz@xxxxxxxxxx>
Cc: Dave Hansen <dave@xxxxxxxxxxxxxxxxxx>,
---
fs/exec.c | 19 ++++++++++++++++++-
include/linux/sched.h | 5 ++---
2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 3d48ac6..bce64bb 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -995,9 +995,26 @@ static void flush_old_files(struct files_struct * files)
spin_unlock(&files->file_lock);
}
+/**
+ * get_task_comm - get task name
+ * @buf: buffer to store result. must be at least sizeof(tsk->comm) in size
+ * @tsk: the task in question
+ *
+ * Note: task->comm has slightly complex locking rule.
+ *
+ * 1) write own or another task's name
+ * -> must use set_task_comm()
+ * 2) read another task's name
+ * -> must use get_task_comm() or take task_lock() manually.
+ * 3) read own task's name
+ * -> recommend to use get_task_comm() or take task_lock() manually.
+ * If you don't take task_lock(), you may see incomplete or empty string.
+ * But it's guaranteed to keep valid C NUL-terminated string.
+ * (ie never be crash)
+ * So, debugging printk may be ok to read it without lock.
+ */
char *get_task_comm(char *buf, struct task_struct *tsk)
{
- /* buf must be at least sizeof(tsk->comm) in size */
task_lock(tsk);
strncpy(buf, tsk->comm, sizeof(tsk->comm));
task_unlock(tsk);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 275c1a1..3e86500 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1334,9 +1334,8 @@ struct task_struct {
struct cred *replacement_session_keyring; /* for KEYCTL_SESSION_TO_PARENT */
char comm[TASK_COMM_LEN]; /* executable name excluding path
- - access with [gs]et_task_comm (which lock
- it with task_lock())
- - initialized normally by setup_new_exec */
+ detailed locking rule is described at
+ get_task_comm() */
/* file system info */
int link_count, total_link_count;
#ifdef CONFIG_SYSVIPC
--
1.7.3.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>