[Patch] mmu_notifiers destroyed by __mmu_notifier_release() retain extra mm_count.

[Patch] mmu_notifiers destroyed by __mmu_notifier_release() retain extra mm_count.

[Robin Holt]

An application relying upon mmu_notifier_release for teardown of the
mmu_notifiers will leak mm_structs.  At the do_mmu_notifier_register
increments mm_count, but __mmu_notifier_release() does not decrement it.

Signed-off-by: Robin Holt <holt@sgi.com>
CC: Stable kernel maintainers <stable@vger.kernel.org>

---

I detected this while running a 2.6.27 kernel.  Could this get added to
the stable trees when accepted as well?  It does cause a denial of
service with OOM.

 mm/mmu_notifier.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6.27/mm/mmu_notifier.c
===================================================================
--- linux-2.6.27.orig/mm/mmu_notifier.c	2008-10-09 17:13:53.000000000 -0500
+++ linux-2.6.27/mm/mmu_notifier.c	2009-02-05 10:55:07.076561592 -0600
@@ -61,6 +61,7 @@ void __mmu_notifier_release(struct mm_st
 		if (mn->ops->release)
 			mn->ops->release(mn, mm);
 		rcu_read_unlock();
+		mmdrop(mm);	/* matches do_mmu_notifier_register's inc */
 		spin_lock(&mm->mmu_notifier_mm->lock);
 	}
 	spin_unlock(&mm->mmu_notifier_mm->lock);

Re: [Patch] mmu_notifiers destroyed by __mmu_notifier_release() retain extra mm_count.

[Christoph Lameter]

The drop of the refcount needs to occur  after the last use of
data in the mmstruct because mmdrop() may free the mmstruct.

Place it after the synchronize_rcu?

Re: [Patch] mmu_notifiers destroyed by __mmu_notifier_release() retain extra mm_count.

[Robin Holt]

On Thu, Feb 05, 2009 at 02:30:29PM -0500, Christoph Lameter wrote:
> The drop of the refcount needs to occur  after the last use of
> data in the mmstruct because mmdrop() may free the mmstruct.

Not this time.  We are being called from process termination and the
calling function is assured to hold one reference count.

We would also have to track how many callouts were made and then do
drops in a loop, but as stated above, I don't think it is needed.

Robin

Re: [Patch] mmu_notifiers destroyed by __mmu_notifier_release() retain extra mm_count.

[Andrea Arcangeli]

On Thu, Feb 05, 2009 at 11:23:03AM -0600, Robin Holt wrote:
> 
> An application relying upon mmu_notifier_release for teardown of the
> mmu_notifiers will leak mm_structs.  At the do_mmu_notifier_register
> increments mm_count, but __mmu_notifier_release() does not decrement it.

Sure agreed, thanks! This got unnoticed this long because KVM uses the
unregister method instead of the self-disarming ->release and I guess
your systems have quite some ram so it'd take a while for the memleak
to trigger.

Re: [Patch] mmu_notifiers destroyed by __mmu_notifier_release() retain extra mm_count.

[Andrew Morton]

On Thu, 5 Feb 2009 11:23:03 -0600
Robin Holt <holt@sgi.com> wrote:

> CC: Stable kernel maintainers <stable@vger.kernel.org>

stable@kernel.org.

The vger address might work, I don't know.

Re: [Patch] mmu_notifiers destroyed by __mmu_notifier_release() retain extra mm_count.

[Christoph Lameter]

On Thu, 5 Feb 2009, Robin Holt wrote:

> On Thu, Feb 05, 2009 at 02:30:29PM -0500, Christoph Lameter wrote:
> > The drop of the refcount needs to occur  after the last use of
> > data in the mmstruct because mmdrop() may free the mmstruct.
>
> Not this time.  We are being called from process termination and the
> calling function is assured to hold one reference count.

Maybe add a comment that says that this is a requirement for the
caller? mmdrop() has logic to free the mmstruct.

One also needs to wonder why we acquire the refcount for the mmu
notifier on the mmstruct at all. Maybe remove the

	atomic_inc()

from mmu_notifier_register() instead? Looks strange there especially since
we have a BUG_ON there as well that verifies that the number of refcount
is already above 0.

How about this patch instead?


Subject: mmu_notifier: Remove superfluous increase of the mm refcount

The mm refcount is handled by the caller of mmu_notifier_register and
mmu_notifier_unregister(). There is no need to increase the refcount.
Increasing the refcount led to a memory leak.

Signed-off-by: Christoph Lameter <cl@linux-foundation.org>

Index: linux-2.6/mm/mmu_notifier.c
===================================================================
--- linux-2.6.orig/mm/mmu_notifier.c	2009-02-05 17:55:27.000000000 -0600
+++ linux-2.6/mm/mmu_notifier.c	2009-02-05 17:55:31.000000000 -0600
@@ -167,7 +167,6 @@
 		mm->mmu_notifier_mm = mmu_notifier_mm;
 		mmu_notifier_mm = NULL;
 	}
-	atomic_inc(&mm->mm_count);

 	/*
 	 * Serialize the update against mmu_notifier_unregister. A

Re: [Patch] mmu_notifiers destroyed by __mmu_notifier_release() retain extra mm_count.

[Andrea Arcangeli]

On Thu, Feb 05, 2009 at 06:54:33PM -0500, Christoph Lameter wrote:
> One also needs to wonder why we acquire the refcount for the mmu
> notifier on the mmstruct at all. Maybe remove the
> 
> 	atomic_inc()
> 
> from mmu_notifier_register() instead? Looks strange there especially since
> we have a BUG_ON there as well that verifies that the number of refcount
> is already above 0.
> 
> How about this patch instead?

Surely you have to remove mmdrop from mmu_notifier_unregister if you
do that. But with the other patch that mmdrop should also be mvoed up
now that I think about it. So both patches looks wrong.


Ok I think the issue here is that with the current code the unregister
call is mandatory to avoid memleak, if you do like KVM does everything
is fine, even if ->release fires through exit_mmap, later unregister
is called when the fd is closed so all works fine then.

The reason of the mm_count pin is to avoid the driver having to
increase mm_count itself _before_ mmu_notifier_register, basically the
mm has to exist as long as any mmu notifier is attached to an mm, if
mm goes away, clearly the notifier list gets corrupted.

It all boils down if unregister is mandatory or not. If it's mandatory
current code is ok, if it's not, then you've to decide if to remove
both mmdrop and atomic_inc and have the caller handle it (which is
likely ok with kvm) or to add mmdrop to the auto-disarming code, and
then move the mmdrop up in the !hlist_unhashed path of unregister
(which was missing from Robin's patch and could trigger a double
mmdrop if one always calls unregister unconditionally which is meant
to be allowed with current code and that's the kvm usage model too).

Re: [Patch] mmu_notifiers destroyed by __mmu_notifier_release() retain extra mm_count.

[Andrea Arcangeli]

On Fri, Feb 06, 2009 at 02:38:05AM +0100, Andrea Arcangeli wrote:
> It all boils down if unregister is mandatory or not. If it's mandatory

Oh I just found I documented it too!! ;)

/*
 * Must not hold mmap_sem nor any other VM related lock when calling
 * this registration function. Must also ensure mm_users can't go down
 * to zero while this runs to avoid races with mmu_notifier_release,
 * so mm has to be current->mm or the mm should be pinned safely such
 * as with get_task_mm(). If the mm is not current->mm, the mm_users
 * pin should be released by calling mmput after mmu_notifier_register
 * returns. mmu_notifier_unregister must be always called to
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 * unregister the notifier. mm_count is automatically pinned to allow
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 * mmu_notifier_unregister to safely run at any time later, before or 
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
* after exit_mmap. ->release will always be called before exit_mmap
 * frees the pages.
 */

So in short the current code has no bugs and the fact you have to call
unregister is intentional. Not patch required unless you request to
change API. If you don't call unregister mm will be leaked,
simply. For a moment I thought unregister wasn't mandatory because at
some point in one of the dozen versions of the api it wasn't, but in
the end I thought having an mm_count auto-pinning leaving no window
for corrupted mmu_notifier list was preferable ;).

Re: [Patch] mmu_notifiers destroyed by __mmu_notifier_release() retain extra mm_count.

[Robin Holt]

On Fri, Feb 06, 2009 at 02:44:00AM +0100, Andrea Arcangeli wrote:
> simply. For a moment I thought unregister wasn't mandatory because at
> some point in one of the dozen versions of the api it wasn't, but in

You are right, I am remembering an older version of the API (which I
still like better, obviously ;) ).  I also see the problems each choice
of API can cause.  I think the current API is the more reasonable
choice.  I have adjusted XPMEM to keep a copy of the mm_struct pointer
at register time with my own accompanying inc of mm_count and likewise
do the unregister and mmdrop();  This resolved my problem.

Sorry for the noise.

Andrew, could you throw this patch as away as quickly as possible.
Sorry for wasting your time.

Thanks,
Robin

Re: [Patch] mmu_notifiers destroyed by __mmu_notifier_release() retain extra mm_count.

[Andrea Arcangeli]

Glad you are ok with mandatory unregister, it wasn't a problem for GRU
either and the auto-mm-pinning gives peace of mind.

On Fri, Feb 06, 2009 at 06:58:45AM -0600, Robin Holt wrote:
> Sorry for the noise.

No prob from my part, one more review and refresh of correctness of
current code didn't hurt ;).