From patchwork Wed May 18 01:41:02 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: John Stultz X-Patchwork-Id: 1513 Return-Path: Delivered-To: unknown Received: from imap.gmail.com (74.125.159.109) by localhost6.localdomain6 with IMAP4-SSL; 08 Jun 2011 14:52:39 -0000 Delivered-To: patches@linaro.org Received: by 10.224.54.134 with SMTP id q6cs52498qag; Tue, 17 May 2011 18:41:20 -0700 (PDT) Received: by 10.236.193.10 with SMTP id j10mr1390890yhn.506.1305682880446; Tue, 17 May 2011 18:41:20 -0700 (PDT) Received: from e34.co.us.ibm.com (e34.co.us.ibm.com [32.97.110.152]) by mx.google.com with ESMTPS id d6si2312022yhe.200.2011.05.17.18.41.20 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 17 May 2011 18:41:20 -0700 (PDT) Received-SPF: pass (google.com: domain of jstultz@us.ibm.com designates 32.97.110.152 as permitted sender) client-ip=32.97.110.152; Authentication-Results: mx.google.com; spf=pass (google.com: domain of jstultz@us.ibm.com designates 32.97.110.152 as permitted sender) smtp.mail=jstultz@us.ibm.com Received: from d03relay01.boulder.ibm.com (d03relay01.boulder.ibm.com [9.17.195.226]) by e34.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p4I1SkKa018557; Tue, 17 May 2011 19:28:46 -0600 Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d03relay01.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p4I1fF1P145168; Tue, 17 May 2011 19:41:15 -0600 Received: from d03av03.boulder.ibm.com (loopback [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p4HJfEGD010192; Tue, 17 May 2011 13:41:15 -0600 Received: from kernel.beaverton.ibm.com (kernel.beaverton.ibm.com [9.47.67.96]) by d03av03.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id p4HJfEfp010189; Tue, 17 May 2011 13:41:14 -0600 Received: by kernel.beaverton.ibm.com (Postfix, from userid 1056) id DD2FD1E750D; Tue, 17 May 2011 18:41:13 -0700 (PDT) From: John Stultz To: LKML Cc: John Stultz , Joe Perches , Ingo Molnar , Michal Nazarewicz , Andy Whitcroft , Jiri Slaby , KOSAKI Motohiro , David Rientjes , Dave Hansen , Andrew Morton , linux-mm@kvack.org Subject: [PATCH 1/4] comm: Introduce comm_lock spinlock to protect task->comm access Date: Tue, 17 May 2011 18:41:02 -0700 Message-Id: <1305682865-27111-2-git-send-email-john.stultz@linaro.org> X-Mailer: git-send-email 1.7.3.2.146.gca209 In-Reply-To: <1305682865-27111-1-git-send-email-john.stultz@linaro.org> References: <1305682865-27111-1-git-send-email-john.stultz@linaro.org> Since my commit 4614a696bd1c3a9af3a08f0e5874830a85b889d4, the task->comm value could be changed by other threads. Thus, the implicit rules for current->comm access being safe without locking are no longer true. Accessing current->comm without holding the task lock may result in null or incomplete strings (however, access won't run off the end of the string). In order to properly fix this, I've introduced a comm_lock spinlock which will protect comm access and modified get_task_comm() and set_task_comm() to use it. Since there are a number of cases where comm access is open-coded safely grabbing the task_lock(), we preserve the task locking in set_task_comm, so those users are also safe. With this patch, users that access current->comm without a lock are still prone to null/incomplete comm strings, but it should be no worse then it is now. The next step is to go through and convert all comm accesses to use get_task_comm(). This is substantial, but can be done bit by bit, reducing the race windows with each patch. CC: Joe Perches CC: Ingo Molnar CC: Michal Nazarewicz CC: Andy Whitcroft CC: Jiri Slaby CC: KOSAKI Motohiro CC: David Rientjes CC: Dave Hansen CC: Andrew Morton CC: linux-mm@kvack.org Acked-by: David Rientjes Reviewed-by: KOSAKI Motohiro Signed-off-by: John Stultz --- fs/exec.c | 19 ++++++++++++++++--- include/linux/init_task.h | 1 + include/linux/sched.h | 5 ++--- kernel/fork.c | 1 + 4 files changed, 20 insertions(+), 6 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 5e62d26..34fa611 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -998,17 +998,28 @@ static void flush_old_files(struct files_struct * files) char *get_task_comm(char *buf, struct task_struct *tsk) { - /* buf must be at least sizeof(tsk->comm) in size */ - task_lock(tsk); + unsigned long flags; + + spin_lock_irqsave(&tsk->comm_lock, flags); strncpy(buf, tsk->comm, sizeof(tsk->comm)); - task_unlock(tsk); + spin_unlock_irqrestore(&tsk->comm_lock, flags); return buf; } void set_task_comm(struct task_struct *tsk, char *buf) { + unsigned long flags; + + /* + * XXX - Even though comm is protected by comm_lock, + * we take the task_lock here to serialize against + * current users that directly access comm. + * Once those users are removed, we can drop the + * task locking & memsetting. + */ task_lock(tsk); + spin_lock_irqsave(&tsk->comm_lock, flags); /* * Threads may access current->comm without holding * the task lock, so write the string carefully. @@ -1018,6 +1029,8 @@ void set_task_comm(struct task_struct *tsk, char *buf) memset(tsk->comm, 0, TASK_COMM_LEN); wmb(); strlcpy(tsk->comm, buf, sizeof(tsk->comm)); + spin_unlock_irqrestore(&tsk->comm_lock, flags); + task_unlock(tsk); perf_event_comm(tsk); } diff --git a/include/linux/init_task.h b/include/linux/init_task.h index caa151f..b69d94b 100644 --- a/include/linux/init_task.h +++ b/include/linux/init_task.h @@ -161,6 +161,7 @@ extern struct cred init_cred; .group_leader = &tsk, \ RCU_INIT_POINTER(.real_cred, &init_cred), \ RCU_INIT_POINTER(.cred, &init_cred), \ + .comm_lock = __SPIN_LOCK_UNLOCKED(tsk.comm_lock), \ .comm = "swapper", \ .thread = INIT_THREAD, \ .fs = &init_fs, \ diff --git a/include/linux/sched.h b/include/linux/sched.h index 18d63ce..f8a7cdf 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1333,10 +1333,9 @@ struct task_struct { const struct cred __rcu *cred; /* effective (overridable) subjective task * credentials (COW) */ struct cred *replacement_session_keyring; /* for KEYCTL_SESSION_TO_PARENT */ - + spinlock_t comm_lock; /* protect's comm */ char comm[TASK_COMM_LEN]; /* executable name excluding path - - access with [gs]et_task_comm (which lock - it with task_lock()) + - access with [gs]et_task_comm - initialized normally by setup_new_exec */ /* file system info */ int link_count, total_link_count; diff --git a/kernel/fork.c b/kernel/fork.c index e7548de..f53bf29 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1080,6 +1080,7 @@ static struct task_struct *copy_process(unsigned long clone_flags, rcu_copy_process(p); p->vfork_done = NULL; spin_lock_init(&p->alloc_lock); + spin_lock_init(&p->comm_lock); init_sigpending(&p->pending);