Commit 751efd86 authored by Robin Holt's avatar Robin Holt Committed by Linus Torvalds

mmu_notifier_unregister NULL Pointer deref and multiple ->release() callouts

There is a race condition between mmu_notifier_unregister() and
__mmu_notifier_release().

Assume two tasks, one calling mmu_notifier_unregister() as a result of a
filp_close() ->flush() callout (task A), and the other calling
mmu_notifier_release() from an mmput() (task B).

                A                               B
t1                                              srcu_read_lock()
t2              if (!hlist_unhashed())
t3                                              srcu_read_unlock()
t4              srcu_read_lock()
t5                                              hlist_del_init_rcu()
t6                                              synchronize_srcu()
t7              srcu_read_unlock()
t8              hlist_del_rcu()  <--- NULL pointer deref.

Additionally, the list traversal in __mmu_notifier_release() is not
protected by the by the mmu_notifier_mm->hlist_lock which can result in
callouts to the ->release() notifier from both mmu_notifier_unregister()
and __mmu_notifier_release().

-stable suggestions:

The stable trees prior to 3.7.y need commits 21a92735 and
70400303 cherry-picked in that order prior to cherry-picking this
commit.  The 3.7.y tree already has those two commits.
Signed-off-by: default avatarRobin Holt <holt@sgi.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Wanpeng Li <liwanp@linux.vnet.ibm.com>
Cc: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
Cc: Avi Kivity <avi@redhat.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Sagi Grimberg <sagig@mellanox.co.il>
Cc: Haggai Eran <haggaie@mellanox.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent c1f19495
...@@ -37,49 +37,51 @@ static struct srcu_struct srcu; ...@@ -37,49 +37,51 @@ static struct srcu_struct srcu;
void __mmu_notifier_release(struct mm_struct *mm) void __mmu_notifier_release(struct mm_struct *mm)
{ {
struct mmu_notifier *mn; struct mmu_notifier *mn;
struct hlist_node *n;
int id; int id;
/* /*
* SRCU here will block mmu_notifier_unregister until * srcu_read_lock() here will block synchronize_srcu() in
* ->release returns. * mmu_notifier_unregister() until all registered
* ->release() callouts this function makes have
* returned.
*/ */
id = srcu_read_lock(&srcu); id = srcu_read_lock(&srcu);
hlist_for_each_entry_rcu(mn, n, &mm->mmu_notifier_mm->list, hlist)
/*
* if ->release runs before mmu_notifier_unregister it
* must be handled as it's the only way for the driver
* to flush all existing sptes and stop the driver
* from establishing any more sptes before all the
* pages in the mm are freed.
*/
if (mn->ops->release)
mn->ops->release(mn, mm);
srcu_read_unlock(&srcu, id);
spin_lock(&mm->mmu_notifier_mm->lock); spin_lock(&mm->mmu_notifier_mm->lock);
while (unlikely(!hlist_empty(&mm->mmu_notifier_mm->list))) { while (unlikely(!hlist_empty(&mm->mmu_notifier_mm->list))) {
mn = hlist_entry(mm->mmu_notifier_mm->list.first, mn = hlist_entry(mm->mmu_notifier_mm->list.first,
struct mmu_notifier, struct mmu_notifier,
hlist); hlist);
/* /*
* We arrived before mmu_notifier_unregister so * Unlink. This will prevent mmu_notifier_unregister()
* mmu_notifier_unregister will do nothing other than * from also making the ->release() callout.
* to wait ->release to finish and
* mmu_notifier_unregister to return.
*/ */
hlist_del_init_rcu(&mn->hlist); hlist_del_init_rcu(&mn->hlist);
spin_unlock(&mm->mmu_notifier_mm->lock);
/*
* Clear sptes. (see 'release' description in mmu_notifier.h)
*/
if (mn->ops->release)
mn->ops->release(mn, mm);
spin_lock(&mm->mmu_notifier_mm->lock);
} }
spin_unlock(&mm->mmu_notifier_mm->lock); spin_unlock(&mm->mmu_notifier_mm->lock);
/* /*
* synchronize_srcu here prevents mmu_notifier_release to * All callouts to ->release() which we have done are complete.
* return to exit_mmap (which would proceed freeing all pages * Allow synchronize_srcu() in mmu_notifier_unregister() to complete
* in the mm) until the ->release method returns, if it was */
* invoked by mmu_notifier_unregister. srcu_read_unlock(&srcu, id);
*
* The mmu_notifier_mm can't go away from under us because one /*
* mm_count is hold by exit_mmap. * mmu_notifier_unregister() may have unlinked a notifier and may
* still be calling out to it. Additionally, other notifiers
* may have been active via vmtruncate() et. al. Block here
* to ensure that all notifier callouts for this mm have been
* completed and the sptes are really cleaned up before returning
* to exit_mmap().
*/ */
synchronize_srcu(&srcu); synchronize_srcu(&srcu);
} }
...@@ -294,31 +296,31 @@ void mmu_notifier_unregister(struct mmu_notifier *mn, struct mm_struct *mm) ...@@ -294,31 +296,31 @@ void mmu_notifier_unregister(struct mmu_notifier *mn, struct mm_struct *mm)
{ {
BUG_ON(atomic_read(&mm->mm_count) <= 0); BUG_ON(atomic_read(&mm->mm_count) <= 0);
spin_lock(&mm->mmu_notifier_mm->lock);
if (!hlist_unhashed(&mn->hlist)) { if (!hlist_unhashed(&mn->hlist)) {
/*
* SRCU here will force exit_mmap to wait ->release to finish
* before freeing the pages.
*/
int id; int id;
id = srcu_read_lock(&srcu);
/* /*
* exit_mmap will block in mmu_notifier_release to * Ensure we synchronize up with __mmu_notifier_release().
* guarantee ->release is called before freeing the
* pages.
*/ */
id = srcu_read_lock(&srcu);
hlist_del_rcu(&mn->hlist);
spin_unlock(&mm->mmu_notifier_mm->lock);
if (mn->ops->release) if (mn->ops->release)
mn->ops->release(mn, mm); mn->ops->release(mn, mm);
srcu_read_unlock(&srcu, id);
spin_lock(&mm->mmu_notifier_mm->lock); /*
hlist_del_rcu(&mn->hlist); * Allow __mmu_notifier_release() to complete.
*/
srcu_read_unlock(&srcu, id);
} else
spin_unlock(&mm->mmu_notifier_mm->lock); spin_unlock(&mm->mmu_notifier_mm->lock);
}
/* /*
* Wait any running method to finish, of course including * Wait for any running method to finish, including ->release() if it
* ->release if it was run by mmu_notifier_relase instead of us. * was run by __mmu_notifier_release() instead of us.
*/ */
synchronize_srcu(&srcu); synchronize_srcu(&srcu);
......
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