linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: David Rientjes <rientjes@google.com>
To: Andrew Morton <akpm@linux-foundation.org>,
	Linus Torvalds <torvalds@linux-foundation.org>
Cc: Dave Jones <davej@redhat.com>,
	KOSAKI Motohiro <kosaki.motohiro@gmail.com>,
	bhutchings@solarflare.com,
	Konstantin Khlebnikov <khlebnikov@openvz.org>,
	Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>,
	Hugh Dickins <hughd@google.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: [patch for-3.7] mm, mempolicy: fix printing stack contents in numa_maps
Date: Tue, 16 Oct 2012 17:31:23 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.00.1210161714110.17278@chino.kir.corp.google.com> (raw)
In-Reply-To: <alpine.DEB.2.00.1210161657220.14014@chino.kir.corp.google.com>

When reading /proc/pid/numa_maps, it's possible to return the contents of 
the stack where the mempolicy string should be printed if the policy gets 
freed from beneath us.

This happens because mpol_to_str() may return an error the 
stack-allocated buffer is then printed without ever being stored.

There are two possible error conditions in mpol_to_str():

 - if the buffer allocated is insufficient for the string to be stored, 
   and

 - if the mempolicy has an invalid mode.

The first error condition is not triggered in any of the callers to 
mpol_to_str(): at least 50 bytes is always allocated on the stack and this 
is sufficient for the string to be written.  A future patch should convert 
this into BUILD_BUG_ON() since we know the maximum strlen possible, but 
that's not -rc material.

The second error condition is possible if a race occurs in dropping a 
reference to a task's mempolicy causing it to be freed during the read().  
The slab poison value is then used for the mode and mpol_to_str() returns 
-EINVAL.

This race is only possible because get_vma_policy() believes that 
mm->mmap_sem protects task->mempolicy, which isn't true.  The exit path 
does not hold mm->mmap_sem when dropping the reference or setting 
task->mempolicy to NULL: it uses task_lock(task) instead.

Thus, it's required for the caller of a task mempolicy to hold 
task_lock(task) while grabbing the mempolicy and reading it.  Callers with 
a vma policy store their mempolicy earlier and can simply increment the 
reference count so it's guaranteed not to be freed.

Reported-by: Dave Jones <davej@redhat.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 fs/proc/task_mmu.c |    7 +++++--
 mm/mempolicy.c     |    5 ++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1158,6 +1158,7 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
 	struct vm_area_struct *vma = v;
 	struct numa_maps *md = &numa_priv->md;
 	struct file *file = vma->vm_file;
+	struct task_struct *task = proc_priv->task;
 	struct mm_struct *mm = vma->vm_mm;
 	struct mm_walk walk = {};
 	struct mempolicy *pol;
@@ -1177,9 +1178,11 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
 	walk.private = md;
 	walk.mm = mm;
 
-	pol = get_vma_policy(proc_priv->task, vma, vma->vm_start);
+	task_lock(task);
+	pol = get_vma_policy(task, vma, vma->vm_start);
 	mpol_to_str(buffer, sizeof(buffer), pol, 0);
 	mpol_cond_put(pol);
+	task_unlock(task);
 
 	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
 
@@ -1189,7 +1192,7 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
 	} else if (vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk) {
 		seq_printf(m, " heap");
 	} else {
-		pid_t tid = vm_is_stack(proc_priv->task, vma, is_pid);
+		pid_t tid = vm_is_stack(task, vma, is_pid);
 		if (tid != 0) {
 			/*
 			 * Thread stack in /proc/PID/task/TID/maps or
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 0b78fb9..d04a8a5 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1536,9 +1536,8 @@ asmlinkage long compat_sys_mbind(compat_ulong_t start, compat_ulong_t len,
  *
  * Returns effective policy for a VMA at specified address.
  * Falls back to @task or system default policy, as necessary.
- * Current or other task's task mempolicy and non-shared vma policies
- * are protected by the task's mmap_sem, which must be held for read by
- * the caller.
+ * Current or other task's task mempolicy and non-shared vma policies must be
+ * protected by task_lock(task) by the caller.
  * Shared policies [those marked as MPOL_F_SHARED] require an extra reference
  * count--added by the get_policy() vm_op, as appropriate--to protect against
  * freeing by another task.  It is the caller's responsibility to free the

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply	other threads:[~2012-10-17  0:31 UTC|newest]

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-10-08 15:09 mpol_to_str revisited Dave Jones
2012-10-08 15:15 ` Dave Jones
2012-10-08 20:46   ` David Rientjes
2012-10-08 20:35 ` David Rientjes
2012-10-08 20:52   ` Dave Jones
2012-10-16  0:48     ` David Rientjes
2012-10-09  0:33 ` Ben Hutchings
2012-10-16  2:34 ` KOSAKI Motohiro
2012-10-16  3:58   ` David Rientjes
2012-10-16  5:10     ` KOSAKI Motohiro
2012-10-16  6:10       ` David Rientjes
2012-10-16 23:39         ` KOSAKI Motohiro
2012-10-17  0:12           ` David Rientjes
2012-10-17  0:31             ` David Rientjes [this message]
2012-10-17  1:38               ` [patch for-3.7] mm, mempolicy: fix printing stack contents in numa_maps KOSAKI Motohiro
2012-10-17  1:49                 ` David Rientjes
2012-10-17  1:53                   ` KOSAKI Motohiro
2012-10-17  4:05               ` Dave Jones
2012-10-17  5:24                 ` David Rientjes
2012-10-17  5:42                   ` Kamezawa Hiroyuki
2012-10-17  8:49                     ` KOSAKI Motohiro
2012-10-17 19:50                       ` David Rientjes
2012-10-17 21:05                         ` KOSAKI Motohiro
2012-10-17 21:27                           ` David Rientjes
2012-10-17 18:14                   ` Dave Jones
2012-10-17 19:21                     ` David Rientjes
2012-10-17 19:32                       ` Dave Jones
2012-10-17 19:38                         ` David Rientjes
2012-10-17 19:45                           ` Dave Jones
2012-10-17 20:28                             ` [patch for-3.7] mm, mempolicy: avoid taking mutex inside spinlock when reading numa_maps David Rientjes
2012-10-17 21:31                               ` [patch for-3.7 v2] " David Rientjes
2012-10-18  4:06                                 ` Kamezawa Hiroyuki
2012-10-18  4:14                                   ` Linus Torvalds
2012-10-18  4:41                                     ` Kamezawa Hiroyuki
2012-10-18  4:34                                   ` Kamezawa Hiroyuki
2012-10-18 20:03                                     ` David Rientjes
2012-10-19  8:35                                       ` [patch for-3.7 v3] mm, mempolicy: hold task->mempolicy refcount while " Kamezawa Hiroyuki
2012-10-19  9:28                                         ` David Rientjes
2012-10-22  2:47                                           ` Kamezawa Hiroyuki
2012-10-22 20:55                                             ` Andrew Morton
2012-10-22 20:56                                             ` David Rientjes
2012-10-19 19:15                                         ` KOSAKI Motohiro
2012-10-19  6:51                                     ` [patch for-3.7 v2] mm, mempolicy: avoid taking mutex inside spinlock when " KOSAKI Motohiro
2012-10-18  4:35                                   ` David Rientjes
2012-10-24 23:30                   ` [patch for-3.7] mm, mempolicy: fix printing stack contents in numa_maps Sasha Levin
2012-10-24 23:34                     ` David Rientjes
2012-10-24 23:37                       ` Sasha Levin
2012-10-25  0:08                         ` David Rientjes
2012-10-25  0:54                           ` KOSAKI Motohiro
2012-10-25  1:15                             ` David Rientjes
2012-10-25 12:19                           ` Peter Zijlstra
2012-10-25 14:39                             ` Peter Zijlstra
2012-10-25 17:23                               ` Sasha Levin
2012-10-25 20:22                               ` David Rientjes
2012-10-25 23:09                               ` Linus Torvalds
2012-10-26  8:48                                 ` Peter Zijlstra
2012-10-31 18:29                                   ` Sasha Levin
2012-11-21  0:59                                     ` Sasha Levin
2012-10-17  1:33             ` mpol_to_str revisited KOSAKI Motohiro

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.00.1210161714110.17278@chino.kir.corp.google.com \
    --to=rientjes@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=bhutchings@solarflare.com \
    --cc=davej@redhat.com \
    --cc=hughd@google.com \
    --cc=kamezawa.hiroyu@jp.fujitsu.com \
    --cc=khlebnikov@openvz.org \
    --cc=kosaki.motohiro@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=n-horiguchi@ah.jp.nec.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox