Thread (10 messages) 10 messages, 4 authors, 2012-02-24

Re: [RFC][PATCH] fix move/migrate_pages() race on task struct

From: Christoph Lameter <hidden>
Date: 2012-02-24 15:20:59
Also in: lkml

On Thu, 23 Feb 2012, Eric W. Biederman wrote:
quoted
The bug in migrate_pages() is that we do a rcu_unlock and a rcu_lock. If
we drop those then we should be safe if the use of a task pointer within a
rcu section is safe without taking a refcount.
Yes the user of a task_struct pointer found via a userspace pid is valid
for the life of an rcu critical section, and the bug is indeed that we
drop the rcu_lock and somehow expect the task to remain valid.

The guarantee comes from release_task.  In release_task we call
__exit_signal which calls __unhash_process, and then we call
delayed_put_task to guarantee that the task lives until the end of the
rcu interval.
Ah. Ok. Great.
In migrate_pages we have a lot of task accesses outside of the rcu
critical section, and without a reference count on task.
Yes but that is only of interesting for setup and verification of
permissions. What matters during migration is that the mm_struct does not
go away and we take a refcount on that one.
I tell you the truth trying to figure out what that code needs to be
correct if task != current makes my head hurt.
Hmm...
I think we need to grab a reference on task_struct, to stop the task
from going away, and in addition we need to hold task_lock.  To keep
task->mm from changing (see exec_mmap).  But we can't do that and sleep
so I think the entire function needs to be rewritten, and the need for
task deep in the migrate_pages path needs to be removed as even with the
reference count held we can race with someone calling exec.
We dont need the task during migration. We only need the mm. The task
is safe until rcu_read_unlock therefore maybe the following should fix
migrate pages:


Subject: migration: Do not do rcu_read_unlock until the last time we need the task_struct pointer

Migration functions perform the rcu_read_unlock too early. As a result the
task pointed to may change. Bugs were introduced when adding security checks
because rcu_unlock/lock sequences were inserted. Plus the security checks
and do_move_pages used the task_struct pointer after rcu_unlock.

Fix those issues by removing the unlock/lock sequences and moving the
rcu_read_unlock after the last use of the task struct pointer.

Signed-off-by: Christoph Lameter <redacted>


---
 mm/mempolicy.c |   22 +++++++++++-----------
 mm/migrate.c   |   28 +++++++++++++++-------------
 2 files changed, 26 insertions(+), 24 deletions(-)

Index: linux-2.6/mm/mempolicy.c
===================================================================
--- linux-2.6.orig/mm/mempolicy.c	2012-01-13 04:04:36.229807226 -0600
+++ linux-2.6/mm/mempolicy.c	2012-02-24 03:11:44.913710625 -0600
@@ -1318,16 +1318,14 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pi
 	rcu_read_lock();
 	task = pid ? find_task_by_vpid(pid) : current;
 	if (!task) {
-		rcu_read_unlock();
 		err = -ESRCH;
-		goto out;
+		goto unlock_out;
 	}
 	mm = get_task_mm(task);
-	rcu_read_unlock();

 	err = -EINVAL;
 	if (!mm)
-		goto out;
+		goto unlock_out;

 	/*
 	 * Check if this process has the right to modify the specified
@@ -1335,33 +1333,31 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pi
 	 * capabilities, superuser privileges or the same
 	 * userid as the target process.
 	 */
-	rcu_read_lock();
 	tcred = __task_cred(task);
 	if (cred->euid != tcred->suid && cred->euid != tcred->uid &&
 	    cred->uid  != tcred->suid && cred->uid  != tcred->uid &&
 	    !capable(CAP_SYS_NICE)) {
-		rcu_read_unlock();
 		err = -EPERM;
-		goto out;
+		goto unlock_out;
 	}
-	rcu_read_unlock();

 	task_nodes = cpuset_mems_allowed(task);
 	/* Is the user allowed to access the target nodes? */
 	if (!nodes_subset(*new, task_nodes) && !capable(CAP_SYS_NICE)) {
 		err = -EPERM;
-		goto out;
+		goto unlock_out;
 	}

 	if (!nodes_subset(*new, node_states[N_HIGH_MEMORY])) {
 		err = -EINVAL;
-		goto out;
+		goto unlock_out;
 	}

 	err = security_task_movememory(task);
 	if (err)
-		goto out;
+		goto unlock_out;

+	rcu_read_unlock();
 	err = do_migrate_pages(mm, old, new,
 		capable(CAP_SYS_NICE) ? MPOL_MF_MOVE_ALL : MPOL_MF_MOVE);
 out:
@@ -1370,6 +1366,10 @@ out:
 	NODEMASK_SCRATCH_FREE(scratch);

 	return err;
+
+unlock_out:
+	rcu_read_unlock();
+	goto out;
 }

Index: linux-2.6/mm/migrate.c
===================================================================
--- linux-2.6.orig/mm/migrate.c	2012-02-06 04:25:35.857094372 -0600
+++ linux-2.6/mm/migrate.c	2012-02-24 03:18:55.569698851 -0600
@@ -1176,20 +1176,17 @@ set_status:
  * Migrate an array of page address onto an array of nodes and fill
  * the corresponding array of status.
  */
-static int do_pages_move(struct mm_struct *mm, struct task_struct *task,
+static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 			 unsigned long nr_pages,
 			 const void __user * __user *pages,
 			 const int __user *nodes,
 			 int __user *status, int flags)
 {
 	struct page_to_node *pm;
-	nodemask_t task_nodes;
 	unsigned long chunk_nr_pages;
 	unsigned long chunk_start;
 	int err;

-	task_nodes = cpuset_mems_allowed(task);
-
 	err = -ENOMEM;
 	pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
 	if (!pm)
@@ -1351,6 +1348,7 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid,
 	struct task_struct *task;
 	struct mm_struct *mm;
 	int err;
+	nodemask_t task_nodes;

 	/* Check flags */
 	if (flags & ~(MPOL_MF_MOVE|MPOL_MF_MOVE_ALL))
@@ -1367,10 +1365,11 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid,
 		return -ESRCH;
 	}
 	mm = get_task_mm(task);
-	rcu_read_unlock();

-	if (!mm)
+	if (!mm) {
+		rcu_read_unlock();
 		return -EINVAL;
+	}

 	/*
 	 * Check if this process has the right to modify the specified
@@ -1378,24 +1377,23 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid,
 	 * capabilities, superuser privileges or the same
 	 * userid as the target process.
 	 */
-	rcu_read_lock();
 	tcred = __task_cred(task);
 	if (cred->euid != tcred->suid && cred->euid != tcred->uid &&
 	    cred->uid  != tcred->suid && cred->uid  != tcred->uid &&
 	    !capable(CAP_SYS_NICE)) {
-		rcu_read_unlock();
 		err = -EPERM;
-		goto out;
+		goto unlock_out;
 	}
-	rcu_read_unlock();

  	err = security_task_movememory(task);
  	if (err)
-		goto out;
+		goto unlock_out;

+	task_nodes = cpuset_mems_allowed(task);
+	rcu_read_unlock();
 	if (nodes) {
-		err = do_pages_move(mm, task, nr_pages, pages, nodes, status,
-				    flags);
+		err = do_pages_move(mm, task_nodes, nr_pages, pages, nodes,
+				status, flags);
 	} else {
 		err = do_pages_stat(mm, nr_pages, pages, status);
 	}
@@ -1403,6 +1401,10 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid,
 out:
 	mmput(mm);
 	return err;
+
+unlock_out:
+	rcu_read_unlock();
+	goto out;
 }

 /*

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help