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).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/