mmu_notifiers destroyed by __mmu_notifier_release() retain extra mm_count.

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.

---

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
===================================================================
@@ -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);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

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.

Index: linux-2.6/mm/mmu_notifier.c
===================================================================
@@ -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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

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).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

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 ;).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

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

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

[email protected].

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>