(2012/10/18 6:31), David Rientjes wrote:
As a result of commit 32f8516a8c73 ("mm, mempolicy: fix printing stack
contents in numa_maps"), the mutex protecting a shared policy can be
inadvertently taken while holding task_lock(task).
Recently, commit b22d127a39dd ("mempolicy: fix a race in
shared_policy_replace()") switched the spinlock within a shared policy to
a mutex so sp_alloc() could block. Thus, a refcount must be grabbed on
all mempolicies returned by get_vma_policy() so it isn't freed while being
passed to mpol_to_str() when reading /proc/pid/numa_maps.
This patch only takes task_lock() while dereferencing task->mempolicy in
get_vma_policy() if it's non-NULL in the lockess check to increment its
refcount. This ensures it will remain in memory until dropped by
__mpol_put() after mpol_to_str() is called.
Refcounts of shared policies are grabbed by the ->get_policy() function of
the vma, all others will be grabbed directly in get_vma_policy(). Now
that this is done, all callers now unconditionally drop the refcount.
please add original problem description....
from your 1st patch.
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.
.....
Hmm, I've read the whole thread again...and, I'm sorry if I misunderstand something.
I think Kosaki mentioned the commit 52cd3b0740. It avoids refcounting in get_vma_policy()
because it's called every time alloc_pages_vma() is called, at every page fault.
So, it seems he doesn't agree this fix because of performance concern on big NUMA,
Can't we have another way to fix ? like this ? too ugly ?
Again, I'm sorry if I misunderstand the points.
==
From bfe7e2ab1c1375b134ec12efce6517149318f75d Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
Date: Thu, 18 Oct 2012 13:17:25 +0900
Subject: [PATCH] hold task->mempolicy while numa_maps scans.
/proc/<pid>/numa_maps scans vma and show mempolicy under
mmap_sem. It sometimes accesses task->mempolicy which can
be freed without mmap_sem and numa_maps can show some
garbage while scanning.
This patch tries to take reference count of task->mempolicy at reading
numa_maps before calling get_vma_policy(). By this, task->mempolicy
will not be freed until numa_maps reaches its end.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
---
fs/proc/task_mmu.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 14df880..d92e868 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -94,6 +94,11 @@ static void vma_stop(struct proc_maps_private *priv, struct vm_area_struct *vma)
{
if (vma && vma != priv->tail_vma) {
struct mm_struct *mm = vma->vm_mm;
+#ifdef CONFIG_NUMA
+ task_lock(priv->task);
+ __mpol_put(priv->task->mempolicy);
+ task_unlock(priv->task);
+#endif
up_read(&mm->mmap_sem);
mmput(mm);
}
@@ -130,6 +135,16 @@ static void *m_start(struct seq_file *m, loff_t *pos)
return mm;
down_read(&mm->mmap_sem);
+ /*
+ * task->mempolicy can be freed even if mmap_sem is down (see kernel/exit.c)
+ * We grab refcount for stable access.
+ * repleacement of task->mmpolicy is guarded by mmap_sem.
+ */
+#ifdef CONFIG_NUMA
+ task_lock(priv->task);
+ mpol_get(priv->task->mempolicy);
+ task_unlock(priv->task);
+#endif
tail_vma = get_gate_vma(priv->task->mm);
priv->tail_vma = tail_vma;
@@ -161,6 +176,11 @@ out:
/* End of vmas has been reached */
m->version = (tail_vma != NULL)? 0: -1UL;
+#ifdef CONFIG_NUMA
+ task_lock(priv->task);
+ __mpol_put(priv->task->mempolicy);
+ task_unlock(priv->task);
+#endif
up_read(&mm->mmap_sem);
mmput(mm);
return tail_vma;
--
1.7.10.2
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>