Commit b22d127a authored by Mel Gorman's avatar Mel Gorman Committed by Linus Torvalds

mempolicy: fix a race in shared_policy_replace()

shared_policy_replace() use of sp_alloc() is unsafe.  1) sp_node cannot
be dereferenced if sp->lock is not held and 2) another thread can modify
sp_node between spin_unlock for allocating a new sp node and next
spin_lock.  The bug was introduced before 2.6.12-rc2.

Kosaki's original patch for this problem was to allocate an sp node and
policy within shared_policy_replace and initialise it when the lock is
reacquired.  I was not keen on this approach because it partially
duplicates sp_alloc().  As the paths were sp->lock is taken are not that
performance critical this patch converts sp->lock to sp->mutex so it can
sleep when calling sp_alloc().

[kosaki.motohiro@jp.fujitsu.com: Original patch]
Signed-off-by: default avatarMel Gorman <mgorman@suse.de>
Acked-by: default avatarKOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Reviewed-by: default avatarChristoph Lameter <cl@linux.com>
Cc: Josh Boyer <jwboyer@gmail.com>
Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent 869833f2
...@@ -188,7 +188,7 @@ struct sp_node { ...@@ -188,7 +188,7 @@ struct sp_node {
struct shared_policy { struct shared_policy {
struct rb_root root; struct rb_root root;
spinlock_t lock; struct mutex mutex;
}; };
void mpol_shared_policy_init(struct shared_policy *sp, struct mempolicy *mpol); void mpol_shared_policy_init(struct shared_policy *sp, struct mempolicy *mpol);
......
...@@ -2083,7 +2083,7 @@ bool __mpol_equal(struct mempolicy *a, struct mempolicy *b) ...@@ -2083,7 +2083,7 @@ bool __mpol_equal(struct mempolicy *a, struct mempolicy *b)
*/ */
/* lookup first element intersecting start-end */ /* lookup first element intersecting start-end */
/* Caller holds sp->lock */ /* Caller holds sp->mutex */
static struct sp_node * static struct sp_node *
sp_lookup(struct shared_policy *sp, unsigned long start, unsigned long end) sp_lookup(struct shared_policy *sp, unsigned long start, unsigned long end)
{ {
...@@ -2147,13 +2147,13 @@ mpol_shared_policy_lookup(struct shared_policy *sp, unsigned long idx) ...@@ -2147,13 +2147,13 @@ mpol_shared_policy_lookup(struct shared_policy *sp, unsigned long idx)
if (!sp->root.rb_node) if (!sp->root.rb_node)
return NULL; return NULL;
spin_lock(&sp->lock); mutex_lock(&sp->mutex);
sn = sp_lookup(sp, idx, idx+1); sn = sp_lookup(sp, idx, idx+1);
if (sn) { if (sn) {
mpol_get(sn->policy); mpol_get(sn->policy);
pol = sn->policy; pol = sn->policy;
} }
spin_unlock(&sp->lock); mutex_unlock(&sp->mutex);
return pol; return pol;
} }
...@@ -2193,10 +2193,10 @@ static struct sp_node *sp_alloc(unsigned long start, unsigned long end, ...@@ -2193,10 +2193,10 @@ static struct sp_node *sp_alloc(unsigned long start, unsigned long end,
static int shared_policy_replace(struct shared_policy *sp, unsigned long start, static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
unsigned long end, struct sp_node *new) unsigned long end, struct sp_node *new)
{ {
struct sp_node *n, *new2 = NULL; struct sp_node *n;
int ret = 0;
restart: mutex_lock(&sp->mutex);
spin_lock(&sp->lock);
n = sp_lookup(sp, start, end); n = sp_lookup(sp, start, end);
/* Take care of old policies in the same range. */ /* Take care of old policies in the same range. */
while (n && n->start < end) { while (n && n->start < end) {
...@@ -2209,16 +2209,14 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start, ...@@ -2209,16 +2209,14 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
} else { } else {
/* Old policy spanning whole new range. */ /* Old policy spanning whole new range. */
if (n->end > end) { if (n->end > end) {
if (!new2) { struct sp_node *new2;
spin_unlock(&sp->lock);
new2 = sp_alloc(end, n->end, n->policy); new2 = sp_alloc(end, n->end, n->policy);
if (!new2) if (!new2) {
return -ENOMEM; ret = -ENOMEM;
goto restart; goto out;
} }
n->end = start; n->end = start;
sp_insert(sp, new2); sp_insert(sp, new2);
new2 = NULL;
break; break;
} else } else
n->end = start; n->end = start;
...@@ -2229,12 +2227,9 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start, ...@@ -2229,12 +2227,9 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
} }
if (new) if (new)
sp_insert(sp, new); sp_insert(sp, new);
spin_unlock(&sp->lock); out:
if (new2) { mutex_unlock(&sp->mutex);
mpol_put(new2->policy); return ret;
kmem_cache_free(sn_cache, new2);
}
return 0;
} }
/** /**
...@@ -2252,7 +2247,7 @@ void mpol_shared_policy_init(struct shared_policy *sp, struct mempolicy *mpol) ...@@ -2252,7 +2247,7 @@ void mpol_shared_policy_init(struct shared_policy *sp, struct mempolicy *mpol)
int ret; int ret;
sp->root = RB_ROOT; /* empty tree == default mempolicy */ sp->root = RB_ROOT; /* empty tree == default mempolicy */
spin_lock_init(&sp->lock); mutex_init(&sp->mutex);
if (mpol) { if (mpol) {
struct vm_area_struct pvma; struct vm_area_struct pvma;
...@@ -2318,7 +2313,7 @@ void mpol_free_shared_policy(struct shared_policy *p) ...@@ -2318,7 +2313,7 @@ void mpol_free_shared_policy(struct shared_policy *p)
if (!p->root.rb_node) if (!p->root.rb_node)
return; return;
spin_lock(&p->lock); mutex_lock(&p->mutex);
next = rb_first(&p->root); next = rb_first(&p->root);
while (next) { while (next) {
n = rb_entry(next, struct sp_node, nd); n = rb_entry(next, struct sp_node, nd);
...@@ -2327,7 +2322,7 @@ void mpol_free_shared_policy(struct shared_policy *p) ...@@ -2327,7 +2322,7 @@ void mpol_free_shared_policy(struct shared_policy *p)
mpol_put(n->policy); mpol_put(n->policy);
kmem_cache_free(sn_cache, n); kmem_cache_free(sn_cache, n);
} }
spin_unlock(&p->lock); mutex_unlock(&p->mutex);
} }
/* assumes fs == KERNEL_DS */ /* assumes fs == KERNEL_DS */
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment