* 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
