linux-kernel - Re: [PATCH 0/4] v6 Improve task->comm locking situation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 20 May 2011 19:41:18 +0900
From:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To:	torvalds@...ux-foundation.org
CC:	john.stultz@...aro.org, linux-kernel@...r.kernel.org,
	joe@...ches.com, mingo@...e.hu, mina86@...a86.com,
	apw@...onical.com, jirislaby@...il.com, rientjes@...gle.com,
	dave@...ux.vnet.ibm.com, akpm@...ux-foundation.org,
	linux-mm@...ck.org
Subject: Re: [PATCH 0/4] v6 Improve task->comm locking situation

(2011/05/19 4:58), Linus Torvalds wrote:
> On Tue, May 17, 2011 at 6:41 PM, John Stultz<john.stultz@...aro.org>  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@...fujitsu.com>
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@...fujitsu.com>
Cc: John Stultz <john.stultz@...aro.org>
Cc: Dave Hansen <dave@...ux.vnet.ibm.com>,
---
  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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/