Re: [PATCH] mm: mmu_notifier: re-fix freed page still mapped in secondary MMU

[Simon Jeons <simon.jeons@gmail.com>]

Hi Robin,
On 04/16/2013 05:31 PM, Robin Holt wrote:
> On Tue, Apr 16, 2013 at 02:39:49PM +0800, Xiao Guangrong wrote:
>> The commit 751efd8610d3 (mmu_notifier_unregister NULL Pointer deref
>> and multiple ->release()) breaks the fix:
>>      3ad3d901bbcfb15a5e4690e55350db0899095a68
>>      (mm: mmu_notifier: fix freed page still mapped in secondary MMU)
> Can you describe how the page is still mapped?  I thought I had all
> cases covered.  Whichever call hits first, I thought we had one callout
> to the registered notifiers.  Are you saying we need multiple callouts?
>
> Also, shouldn't you be asking for a revert commit and then supply a
> subsequent commit for the real fix?  I thought that was the process for
> doing a revert.

mmu_notifier is used for sync normal pte and spte, correct?

>
> Thanks,
> Robin Holt
>
>> This patch reverts the commit and simply fix the bug spotted
>> by that patch
>>
>> This bug is spotted by commit 751efd8610d3:
>> ======
>> 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.
>> ======
>>
>> This can be fixed by using hlist_del_init_rcu instead of hlist_del_rcu.
>>
>> The another issue spotted in the commit is
>> "multiple ->release() callouts", we needn't care it too much because
>> it is really rare (e.g, can not happen on kvm since mmu-notify is unregistered
>> after exit_mmap()) and the later call of multiple ->release should be
>> fast since all the pages have already been released by the first call.
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>> ---
>>   mm/mmu_notifier.c |   81 +++++++++++++++++++++++++++--------------------------
>>   1 files changed, 41 insertions(+), 40 deletions(-)
>>
>> diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
>> index be04122..606777a 100644
>> --- a/mm/mmu_notifier.c
>> +++ b/mm/mmu_notifier.c
>> @@ -40,48 +40,45 @@ void __mmu_notifier_release(struct mm_struct *mm)
>>   	int id;
>>
>>   	/*
>> -	 * srcu_read_lock() here will block synchronize_srcu() in
>> -	 * mmu_notifier_unregister() until all registered
>> -	 * ->release() callouts this function makes have
>> -	 * returned.
>> +	 * SRCU here will block mmu_notifier_unregister until
>> +	 * ->release returns.
>>   	 */
>>   	id = srcu_read_lock(&srcu);
>> +	hlist_for_each_entry_rcu(mn, &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);
>>   	while (unlikely(!hlist_empty(&mm->mmu_notifier_mm->list))) {
>>   		mn = hlist_entry(mm->mmu_notifier_mm->list.first,
>>   				 struct mmu_notifier,
>>   				 hlist);
>> -
>>   		/*
>> -		 * Unlink.  This will prevent mmu_notifier_unregister()
>> -		 * from also making the ->release() callout.
>> +		 * We arrived before mmu_notifier_unregister so
>> +		 * mmu_notifier_unregister will do nothing other than
>> +		 * to wait ->release to finish and
>> +		 * mmu_notifier_unregister to return.
>>   		 */
>>   		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);
>>
>>   	/*
>> -	 * All callouts to ->release() which we have done are complete.
>> -	 * Allow synchronize_srcu() in mmu_notifier_unregister() to complete
>> -	 */
>> -	srcu_read_unlock(&srcu, id);
>> -
>> -	/*
>> -	 * 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 here prevents mmu_notifier_release to
>> +	 * return to exit_mmap (which would proceed freeing all pages
>> +	 * in the mm) until the ->release method returns, if it was
>> +	 * invoked by mmu_notifier_unregister.
>> +	 *
>> +	 * The mmu_notifier_mm can't go away from under us because one
>> +	 * mm_count is hold by exit_mmap.
>>   	 */
>>   	synchronize_srcu(&srcu);
>>   }
>> @@ -292,31 +289,35 @@ void mmu_notifier_unregister(struct mmu_notifier *mn, struct mm_struct *mm)
>>   {
>>   	BUG_ON(atomic_read(&mm->mm_count) <= 0);
>>
>> -	spin_lock(&mm->mmu_notifier_mm->lock);
>>   	if (!hlist_unhashed(&mn->hlist)) {
>> +		/*
>> +		 * SRCU here will force exit_mmap to wait ->release to finish
>> +		 * before freeing the pages.
>> +		 */
>>   		int id;
>>
>> +		id = srcu_read_lock(&srcu);
>>   		/*
>> -		 * Ensure we synchronize up with __mmu_notifier_release().
>> +		 * exit_mmap will block in mmu_notifier_release to
>> +		 * 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)
>>   			mn->ops->release(mn, mm);
>> +		srcu_read_unlock(&srcu, id);
>>
>> +		spin_lock(&mm->mmu_notifier_mm->lock);
>>   		/*
>> -		 * Allow __mmu_notifier_release() to complete.
>> +		 * Can not use list_del_rcu() since __mmu_notifier_release
>> +		 * can delete it before we hold the lock.
>>   		 */
>> -		srcu_read_unlock(&srcu, id);
>> -	} else
>> +		hlist_del_init_rcu(&mn->hlist);
>>   		spin_unlock(&mm->mmu_notifier_mm->lock);
>> +	}
>>
>>   	/*
>> -	 * Wait for any running method to finish, including ->release() if it
>> -	 * was run by __mmu_notifier_release() instead of us.
>> +	 * Wait any running method to finish, of course including
>> +	 * ->release if it was run by mmu_notifier_relase instead of us.
>>   	 */
>>   	synchronize_srcu(&srcu);
>>
>> -- 
>> 1.7.7.6
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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