public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Fix race in process_vm_rw_core
@ 2012-01-13 11:30 Christopher Yeoh
  2012-01-13 16:04 ` Oleg Nesterov
  2012-01-13 22:30 ` Andrew Morton
  0 siblings, 2 replies; 8+ messages in thread
From: Christopher Yeoh @ 2012-01-13 11:30 UTC (permalink / raw)
  To: linux-kernel, Oleg Nesterov, Linus Torvalds; +Cc: Andrew Morton, David Howells

Hi Linus,

Below is a patch which fixes the race in process_vm_core found by
Oleg (http://article.gmane.org/gmane.linux.kernel/1235667/).
It consolidates some code with mm_for_maps since what they do is almost
identical.

Oleg - I've kept the breakout of ptrace_may_attach and get_task_mm to
preserve only having to take the task lock once. I see some performance
difference with a microbenchmark but haven't had a chance to test with
some HPC benchmarks yet so for the moment I'd like to leave it in. At
this stage I think its more important to get the race fixed and I'm at
Linux.conf.au all next week. I'll send a patch out for the 
rw_copy_check_uvector cleanup after I get back from LCA.

Regards,

Chris
-- 
cyeoh@au.ibm.com
Signed-off-by: Chris Yeoh <yeohc@au1.ibm.com>
Cc: stable@vger.kernel.org
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 851ba3d..094d650 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -254,22 +254,7 @@ static struct mm_struct *check_mem_permission(struct task_struct *task)
 
 struct mm_struct *mm_for_maps(struct task_struct *task)
 {
-	struct mm_struct *mm;
-	int err;
-
-	err =  mutex_lock_killable(&task->signal->cred_guard_mutex);
-	if (err)
-		return ERR_PTR(err);
-
-	mm = get_task_mm(task);
-	if (mm && mm != current->mm &&
-			!ptrace_may_access(task, PTRACE_MODE_READ)) {
-		mmput(mm);
-		mm = ERR_PTR(-EACCES);
-	}
-	mutex_unlock(&task->signal->cred_guard_mutex);
-
-	return mm;
+	return get_check_task_mm(task, PTRACE_MODE_READ);
 }
 
 static int proc_pid_cmdline(struct task_struct *task, char * buffer)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 1c4f3e9..8a64cae 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2235,6 +2235,10 @@ static inline void mmdrop(struct mm_struct * mm)
 extern void mmput(struct mm_struct *);
 /* Grab a reference to a task's mm, if it is not already going away */
 extern struct mm_struct *get_task_mm(struct task_struct *task);
+/* Grab a reference to a task's mm, if it is not already going away
+   and ptrace_may_access with the mode parameter passed to it succeeds */
+extern struct mm_struct *get_check_task_mm(struct task_struct *task,
+					   unsigned int mode);
 /* Remove the current tasks stale references to the old mm_struct */
 extern void mm_release(struct task_struct *, struct mm_struct *);
 /* Allocate a new mm structure and copy contents from tsk->mm */
diff --git a/kernel/fork.c b/kernel/fork.c
index da4a6a1..9688fb0 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -644,6 +644,37 @@ struct mm_struct *get_task_mm(struct task_struct *task)
 }
 EXPORT_SYMBOL_GPL(get_task_mm);
 
+struct mm_struct *get_check_task_mm(struct task_struct *task, unsigned int mode)
+{
+	struct mm_struct *mm;
+	int err;
+
+	err =  mutex_lock_killable(&task->signal->cred_guard_mutex);
+	if (err)
+		return ERR_PTR(err);
+
+	task_lock(task);
+	if (__ptrace_may_access(task, mode)) {
+		mm = ERR_PTR(-EACCES);
+		goto out;
+	}
+
+	mm = task->mm;
+	if (mm) {
+		if (task->flags & PF_KTHREAD)
+			mm = NULL;
+		else
+			atomic_inc(&mm->mm_users);
+	}
+
+out:
+	task_unlock(task);
+	mutex_unlock(&task->signal->cred_guard_mutex);
+
+	return mm;
+}
+EXPORT_SYMBOL_GPL(get_check_task_mm);
+
 /* Please note the differences between mmput and mm_release.
  * mmput is called whenever we stop holding onto a mm_struct,
  * error success whatever.
diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
index e920aa3..aa8009d 100644
--- a/mm/process_vm_access.c
+++ b/mm/process_vm_access.c
@@ -298,23 +298,15 @@ static ssize_t process_vm_rw_core(pid_t pid, const struct iovec *lvec,
 		goto free_proc_pages;
 	}
 
-	task_lock(task);
-	if (__ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
-		task_unlock(task);
-		rc = -EPERM;
-		goto put_task_struct;
-	}
-	mm = task->mm;
-
-	if (!mm || (task->flags & PF_KTHREAD)) {
-		task_unlock(task);
-		rc = -EINVAL;
+	mm = get_check_task_mm(task, PTRACE_MODE_ATTACH);
+	if (!mm || IS_ERR(mm)) {
+		if (!mm)
+			rc = -EINVAL;
+		else
+			rc = -EPERM;
 		goto put_task_struct;
 	}
 
-	atomic_inc(&mm->mm_users);
-	task_unlock(task);
-
 	for (i = 0; i < riovcnt && iov_l_curr_idx < liovcnt; i++) {
 		rc = process_vm_rw_single_vec(
 			(unsigned long)rvec[i].iov_base, rvec[i].iov_len,


^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2012-01-16 19:05 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-13 11:30 [PATCH] Fix race in process_vm_rw_core Christopher Yeoh
2012-01-13 16:04 ` Oleg Nesterov
2012-01-13 23:26   ` Christopher Yeoh
2012-01-14 17:58     ` Oleg Nesterov
2012-01-16  2:56       ` Christopher Yeoh
2012-01-16 18:59         ` Oleg Nesterov
2012-01-13 22:30 ` Andrew Morton
2012-01-13 23:30   ` Christopher Yeoh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox