Re: [BUG 3.0.0-rc1] ksm: NULL pointer dereference in ksm_do_scan()

[Andrea Arcangeli <aarcange@redhat.com>]

On Thu, Jun 02, 2011 at 08:36:41AM -0700, Chris Wright wrote:
> * Andrea Arcangeli (aarcange@redhat.com) wrote:
> > On Thu, Jun 02, 2011 at 07:31:43AM -0700, Chris Wright wrote:
> > > * CAI Qian (caiqian@redhat.com) wrote:
> > > > madvise(0x2210000, 4096, 0xc /* MADV_??? */) = 0
> > > > --- SIGSEGV (Segmentation fault) @ 0 (0) ---
> > > 
> > > Right, that's just what the program is trying to do, segfault.
> > > 
> > > > +++ killed by SIGSEGV (core dumped) +++
> > > > Segmentation fault (core dumped)
> > > > 
> > > > Did I miss anything?
> > > 
> > > I found it works but not 100% of the time.
> > > 
> > > So I just run the bug in a loop.
> > 
> > echo 0 >scan_millisecs helps.
> 
> BTW, here's my stack trace (I dropped back to 2.6.39 just to see if it
> happened to be recent regression).  It looks like mm_slot is off the list:
> 
> R10: dead000000200200 R11: dead000000100100

Yes it had to be use after free.

I cooked this patch, still untested but it builds. Will test it soon.

===
Subject: ksm: fix __ksm_exit vs ksm scan SMP race

From: Andrea Arcangeli <aarcange@redhat.com>

If the KSM scan releases the ksm_mmlist_lock after the mm_users already dropped
to zero but before __ksm_exit had a chance runs, both the KSM scan and
__ksm_exit will free the slot. This fixes the SMP race condition by using
test_and_bit_set in __ksm_exit to see if __ksm_exit arrived before the KSM
scan or not.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---

diff --git a/mm/ksm.c b/mm/ksm.c
index d708b3e..47ef4c1 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -645,10 +645,16 @@ static int unmerge_and_remove_all_rmap_items(void)
 		if (ksm_test_exit(mm)) {
 			hlist_del(&mm_slot->link);
 			list_del(&mm_slot->mm_list);
+			/*
+			 * After releasing ksm_mmlist_lock __ksm_exit
+			 * can run and we already changed mm_slot, so
+			 * notify it with MMF_VM_MERGEABLE not to free
+			 * this again.
+			 */
+			clear_bit(MMF_VM_MERGEABLE, &mm->flags);
 			spin_unlock(&ksm_mmlist_lock);
 
 			free_mm_slot(mm_slot);
-			clear_bit(MMF_VM_MERGEABLE, &mm->flags);
 			up_read(&mm->mmap_sem);
 			mmdrop(mm);
 		} else {
@@ -1377,10 +1383,15 @@ next_mm:
 		 */
 		hlist_del(&slot->link);
 		list_del(&slot->mm_list);
+		/*
+		 * After releasing ksm_mmlist_lock __ksm_exit can run
+		 * and we already changed mm_slot, so notify it with
+		 * MMF_VM_MERGEABLE not to free this again.
+		 */
+		clear_bit(MMF_VM_MERGEABLE, &mm->flags);
 		spin_unlock(&ksm_mmlist_lock);
 
 		free_mm_slot(slot);
-		clear_bit(MMF_VM_MERGEABLE, &mm->flags);
 		up_read(&mm->mmap_sem);
 		mmdrop(mm);
 	} else {
@@ -1463,6 +1474,11 @@ int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
 				 VM_NONLINEAR | VM_MIXEDMAP | VM_SAO))
 			return 0;		/* just ignore the advice */
 
+		/*
+		 * It should be safe to test_bit instead of
+		 * test_and_bit_set because the madvise generic caller
+		 * holds the mmap_sem write mode.
+		 */
 		if (!test_bit(MMF_VM_MERGEABLE, &mm->flags)) {
 			err = __ksm_enter(mm);
 			if (err)
@@ -1511,6 +1527,10 @@ int __ksm_enter(struct mm_struct *mm)
 	list_add_tail(&mm_slot->mm_list, &ksm_scan.mm_slot->mm_list);
 	spin_unlock(&ksm_mmlist_lock);
 
+	/*
+	 * It should be safe to set it outside ksm_mmlist_lock because
+	 * we hold a mm_user pin on the mm so __ksm_exit can't run.
+	 */
 	set_bit(MMF_VM_MERGEABLE, &mm->flags);
 	atomic_inc(&mm->mm_count);
 
@@ -1538,9 +1558,28 @@ void __ksm_exit(struct mm_struct *mm)
 	mm_slot = get_mm_slot(mm);
 	if (mm_slot && ksm_scan.mm_slot != mm_slot) {
 		if (!mm_slot->rmap_list) {
-			hlist_del(&mm_slot->link);
-			list_del(&mm_slot->mm_list);
-			easy_to_free = 1;
+			/*
+			 * If MMF_VM_MERGEABLE isn't set it was freed
+			 * by the scan immediately after mm_count
+			 * reached zero (visible by the scan) but
+			 * before __ksm_exit() run, so we don't need
+			 * to do anything here. We don't even need to
+			 * wait for the KSM scan to release the
+			 * mmap_sem as it's not working on the mm
+			 * anymore but it's just releasing it, and it
+			 * probably already did and dropped its
+			 * mm_count too (it would however be safe to
+			 * take mmap_sem here even if MMF_VM_MERGEABLE
+			 * is already clear, as the actual mm can't be
+			 * freed until we return and we run mmdrop
+			 * too, but it's unnecessary).
+			 */
+			if (test_and_clear_bit(MMF_VM_MERGEABLE, &mm->flags)) {
+				hlist_del(&mm_slot->link);
+				list_del(&mm_slot->mm_list);
+				easy_to_free = 1;
+			} else
+				mm_slot = NULL;
 		} else {
 			list_move(&mm_slot->mm_list,
 				  &ksm_scan.mm_slot->mm_list);
@@ -1550,7 +1589,6 @@ void __ksm_exit(struct mm_struct *mm)
 
 	if (easy_to_free) {
 		free_mm_slot(mm_slot);
-		clear_bit(MMF_VM_MERGEABLE, &mm->flags);
 		mmdrop(mm);
 	} else if (mm_slot) {
 		down_write(&mm->mmap_sem);