Re: [PATCH] mm/ksm: Fix race with ksm_exit() in VMA iteration

[Liam R. Howlett]

* David Hildenbrand <david@xxxxxxxxxx> [230308 11:46]:
> On 08.03.23 17:19, Liam R. Howlett wrote:
> > * David Hildenbrand <david@xxxxxxxxxx> [230308 04:41]:
> > > On 07.03.23 21:59, Liam R. Howlett wrote:
> > > > ksm_exit() may remove the mm from the ksm_scan between the unlocking of
> > > > the ksm_mmlist and the start of the VMA iteration.  This results in the
> > > > mmap_read_lock() not being taken and a report from lockdep that the mm
> > > > isn't locked in the maple tree code.
> > > 
> > > I'm confused.
> > 
> > Thanks for looking at this.  My explanation is incorrect.
> > 
> 
> Heh, so that explains my confusion :)
> 
> > > The code does
> > > 
> > > mmap_read_lock(mm);
> > > ...
> > > for_each_vma(vmi, vma) {
> > > mmap_read_unlock(mm);
> > > 
> > > How can we not take the mmap_read_lock() ? Or am I staring at the wrong
> > > mmap_read_lock() ?
> > 
> > That's the right one.  The mmap lock is taken, but the one we are
> > checking is not the correct one.  Let me try again.
> > 
> > Checking the mm struct against the one in the vmi confirms they are the
> > same, so lockdep is telling us the lock we took doesn't match what it
> > expected.  I verified that the lock is the same before the
> > 'for_each_vma()' call by inserting a BUG_ON() which is never triggered
> > with the reproducer.
> > 
> > ksm_test_exit() uses the mm->mm_users atomic to detect an mm exit.  This
> > is usually done in mmget(), mmput(), and friends.
> > 
> > __ksm_exit() and unmerge_and_remove_all_rmap_items() handle freeing by
> > use of the mm->mm_count atomic. This is usually via mmgrab() and mmdrop().
> > 
> > mmput() will call __mmput() if mm_users is decremented to zero.
> > __mmput() calls mmdrop() after the ksm_exit() and then continue with
> > teardown.
> > 
> > So, I believe what is happening is that the external lock flag is being
> > cleared from the maple tree (the one lockdep checks) before we call the
> > iterator.
> > 
> 
> Thanks for the explanation.
> 
> So, IIUC, we are really only fixing  a lockdep issue, assuming that the
> maple tree cleanup code leaves the maple tree in a state where an iterator
> essentially exits right away. Further, I assume this wasn't a problem before
> the maple tree: there would simply be no VMAs to iterate.

Yes, the tree is empty so it will be a noop after the dereference.

This is really just a lockdep fix so I don't think it mattered before.

> 
> > task 1					task 2
> > unmerge_and_remove_all_rmap_items()
> >   spin_lock(&ksm_mmlist_lock);
> >   ksm_scan.mm_slot is set
> >   spin_unlock(&ksm_mmlist_lock);
> > 
> > =======================================================================
> > 	At this point mm->mm_users is 0, but mm_count is not as it will
> > 	be decremented at the end of __mmput().
> > =======================================================================
> > 
> > 					__mmput()
> >   					 ksm_exit()
> > 					  __ksm_exit()
> > 					   spin_lock(&ksm_mmlist_lock);
> > 					   mm_slot is set
> > 					   spin_unlock(&ksm_mmlist_lock)
> > 					   mm_slot == ksm_scan.mm_slot
> > 					   mmap_write_lock();
> > 					   mmap_write_unlock();
> > 					   return
> > 					 exit_mmap()
> > 					   ...
> > 					   mmap_write_lock();
> > 					   __mt_destory()
> > 					     Free all maple tree nodes
> > 					     mt->flags = 0;
> > 					   mmap_write_unlock();
> > 					   ...
> > 
> >   mmap_read_lock()
> >   for_each_vma()
> >     lockdep checks *internal* spinlock
> > 
> > 
> > This was fine before the change as the previous for loop would not have
> > checked the locking and would have hit the ksm_test_exit() test before
> > any problem arose.
> > 
> > Now we are getting a lockdep warning because the maple tree flag for the
> > external lock is cleared.
> > 
> > How about this as the start to the commit message:
> > 
> > The VMA iterator may trigger a lockdep warning if the mm is in the
> > process of being cleaned up before obtaining the mmap_read_lock().
> 
> Maybe something like the following (matches my understanding, as an
> inspiration):
> 
> "
> exit_mmap() will tear down the VMAs (maple tree) with the mmap_lock held in
> write mode. Once we take the mmap_lock in read mode in
> unmerge_and_remove_all_rmap_items(), we are protected against such
> concurrent teardown, however, the teardown might already have happened just
> the way KSM slot registration machinery works.
> 
> Without the VMA iterator, we didn't care. But with the VMA iterator, lockdep
> will now complain when stumbling over a the destroyed maple tree.
> 
> Let's check for the teardown by relying on ksm_test_exit() earlier, before
> working on a torn down maple tree.
> "

I'll give it a shot.

> 
> > 
> > > 
> > > > 
> > > > Fix the race by checking if this mm has been removed before iterating
> > > > the VMAs. __ksm_exit() uses the mmap lock to synchronize the freeing of
> > > > an mm, so it is safe to keep iterating over the VMAs when it is going to
> > > > be freed.
> > > > 
> > > > This change will slow down the mm exit during the race condition, but
> > > > will speed up the non-race scenarios iteration over the VMA list, which
> > > > should be much more common.
> > > 
> > > Would leaving the existing check in help to just stop scanning faster in
> > > that case?
> > 
> > Yes.  But why?  We would stop the scanning faster in the race condition
> > case, but slow the normal case down.
> > 
> > This check was here to ensure that the mm isn't being torn down while
> > it's iterating over the loop.  Hugh (Cc'ed) added this in 2009, but the
> > fundamental problem he specifies in his commit message in 9ba692948008
> > ("ksm: fix oom deadlock") is that exit_mmap() does not take the
> > mmap_lock() - which is no longer the case.  We are safe to iterate the
> > VMAs with the mmap_read_lock() as the mmap_write_lock() is taken during
> > tear down of the VMA tree today.
> > 
> 
> Right. I just spotted that we have a ksm_test_exit() already in
> unmerge_ksm_pages(), so that should be sufficient to make us stop scanning
> in case ksm_exit() is waiting for the mmap lock.

Yeah, I don't think it's necessary in this case but that function is
used elsewhere.

> 
> 
> Adding a comment summarizing why that's required before iterating would be
> nice. Like
> 
> /* Exit right away if the maple tree might have been torn down. */

Ack.

> 
> 
> With a better description, feel free to add
> 
> Acked-by: David Hildenbrand <david@xxxxxxxxxx>

Will do.  I'll give Hugh some time to look at this before sending out a
v2.

Thanks,
Liam