fix swapin race condition

Hi Hugh,

can you review this patch? This is only theoretical so far.

Basically a thread can get stuck in ksm_does_need_to_copy after the unlock_page (with orig_pte pointing to swap entry A). In the meantime another thread can do the ksm swapin copy too, the copy can be swapped out to swap entry B, then a new swapin can create a new copy that can return to swap entry A if reused by things like try_to_free_swap (to me it seems the page pin is useless to prevent the swapcache to go away, it only helps in page reclaim but swapcache is removed by other things too). So then the first thread can finish the ksm-copy find the pte_same pointing to swap entry A again (despite it passed through swap entry B) and break.

I also don't seem to see a guarantee that lookup_swap_cache returns swapcache until we take the lock on the page.

I exclude this can cause regressions, but I'd like to know if it really can happen or if I'm missing something and it cannot happen. I surely looks weird that lookup_swap_cache might return a page that gets removed from swapcache before we take the page lock but I don't see anything preventing it. Surely it's not going to happen until >50% swap is full.

It's also possible to fix it by forcing do_wp_page to run but for a little while it won't be possible to rmap the the instantiated page so I didn't change that even if it probably would make life easier to memcg swapin handlers (maybe, dunno).

Thanks,
Andrea

======
Subject: fix swapin race condition

From: Andrea Arcangeli <[email protected]>

The pte_same check is reliable only if the swap entry remains pinned (by the page lock on swapcache). We've also to ensure the swapcache isn't removed before we take the lock as try_to_free_swap won't care about the page pin.

Signed-off-by Andrea Arcangeli <[email protected]>

---

include/linux/ksm.h +1 -3

 #ifdef CONFIG_KSM
 int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
 		unsigned long end, int advice, unsigned long *vm_flags);

@@ -70,19 +73,14 @@ static inline void set_page_stable_node(
  * We'd like to make this conditional on vma->vm_flags & VM_MERGEABLE,
  * but what if the vma was unmerged while the page was swapped out?
  */
-struct page *ksm_does_need_to_copy(struct page *page,
-			struct vm_area_struct *vma, unsigned long address);
-static inline struct page *ksm_might_need_to_copy(struct page *page,
+static inline int ksm_might_need_to_copy(struct page *page,
 			struct vm_area_struct *vma, unsigned long address)
 {
 	struct anon_vma *anon_vma = page_anon_vma(page);
 

include/linux/ksm.h +2 -2

 int page_referenced_ksm(struct page *page,

@@ -115,10 +113,10 @@ static inline int ksm_madvise(struct vm_
 	return 0;
 }
 
-static inline struct page *ksm_might_need_to_copy(struct page *page,
+static inline int ksm_might_need_to_copy(struct page *page,
 			struct vm_area_struct *vma, unsigned long address)
 {
-	return page;
+	return 0;
 }
 
 static inline int page_referenced_ksm(struct page *page,

mm/ksm.c +0 -3

@@ -1518,8 +1518,6 @@ struct page *ksm_does_need_to_copy(struc
 {
 	struct page *new_page;
 
-	unlock_page(page);	/* any racers will COW it, not modify it */
-
 	new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, address);
 	if (new_page) {
 		copy_user_highpage(new_page, page, address, vma);

@@ -1535,7 +1533,6 @@ struct page *ksm_does_need_to_copy(struc
 			add_page_to_unevictable_list(new_page);
 	}
 
-	page_cache_release(page);
 	return new_page;
 }
 

mm/memory.c +0 -0

 	swp_entry_t entry;
 	pte_t pte;
 	struct mem_cgroup *ptr = NULL;

@@ -2671,10 +2671,23 @@ static int do_swap_page(struct mm_struct
 	lock_page(page);
 	delayacct_clear_flag(DELAYACCT_PF_SWAPIN);
 

mm/memory.c +4 -0

+
+	if (ksm_might_need_to_copy(page, vma, address)) {
+		swapcache = page;
+		page = ksm_does_need_to_copy(page, vma, address);

mm/memory.c +12 -0

@@ -2725,6 +2738,18 @@ static int do_swap_page(struct mm_struct
 	if (vm_swap_full() || (vma->vm_flags & VM_LOCKED) || PageMlocked(page))
 		try_to_free_swap(page);
 	unlock_page(page);
+	if (swapcache) {
+		/*
+		 * Hold the lock to avoid the swap entry to be reused
+		 * until we take the PT lock for the pte_same() check
+		 * (to avoid false positives from pte_same). For
+		 * further safety release the lock after the swap_free
+		 * so that the swap count won't change under a
+		 * parallel locked swapcache.
+		 */
+		unlock_page(swapcache);
+		page_cache_release(swapcache);
+	}
 
 	if (flags & FAULT_FLAG_WRITE) {
 		ret |= do_wp_page(mm, vma, address, page_table, pmd, ptl, pte);

There's a related bug of mine lurking here, only realized in looking through this, which you might want to fix at the same time: I should have moved the PageUptodate check from after the pte_same check to before the ksm_might_need_to_copy, shouldn't I?  As it stands, we might copy junk from an invalid !Uptodate page into a clean new page.

include/linux/ksm.h static inline void set_page_stable_node( · line 84

Quoted 12 lines

  	struct anon_vma *anon_vma = page_anon_vma(page);

include/linux/ksm.h +3 -6

-	if (!anon_vma ||
-	    (anon_vma->root == vma->anon_vma->root&&
-	     page->index == linear_page_index(vma, address)))
-		return page;
-
-	return ksm_does_need_to_copy(page, vma, address);
+	return anon_vma&&
+		(anon_vma->root != vma->anon_vma->root ||
+		 page->index != linear_page_index(vma, address));
  }

Hiding in here is a bigger question than your concern:
are these tests right since Rik refactored the anon_vmas? I just don't know, but hope you and Rik can answer.

Yes, nice find, you're absolutely right: not likely, but possible. Swap is slippery stuff, and that pte_same does depend on keeping the original page locked (or else an additional swap_duplicate+swap_free).

It is well established that by the time lookup_swap_cache() returns, the page it returns may already have been removed from swapcache:
yes, you have to get page lock to be sure. Long ago I put a comment on that into lookup_swap_cache(), but it fell out of the 2.6 version when we briefly changed how that one worked.

It can even happen when swap is near empty: through swapoff, or through reuse_swap_page(), at least.

I'm not aware of any bug we have from that, but sure, it comes as a surprise when you realize it.

Quoted 4 lines

It's also possible to fix it by forcing do_wp_page to run but for a little while it won't be possible to rmap the the instantiated page so I didn't change that even if it probably would make life easier to memcg swapin handlers (maybe, dunno).

That's an interesting idea. I'm not clear what you have in mind there, but if we could get rid of ksm_does_need_to_copy(), letting do_wp_page() do the copy instead, that would be very satisfying. However, I suspect it would rather involve tricking do_wp_page() into doing it, involve a number of hard-to-maintain hacks, appealing to me but to nobody else!

Quoted 5 lines · Hugh Dickins

It is well established that by the time lookup_swap_cache() returns, the page it returns may already have been removed from swapcache:
yes, you have to get page lock to be sure. Long ago I put a comment on that into lookup_swap_cache(), but it fell out of the 2.6 version when we briefly changed how that one worked.

I found it by code review only, so it was unexpected to me. I assume it was unexpected to do_swap_cache as well if you agree this is a bug too and it could trigger in theory.

In the old days I'm quite sure lookup_swap_cache wouldn't return a page evicted by swapcache I think,

but this looks good so we can run
try_to_free_swap and release swap space, without care of the page pins and we decouple the actual pinning of the page from keeping the swap entry pinned too. Just unexpected to me when reading do_swap_page, while being used to the "secular" semantics of lookup_swap_cache ;).

I also thought maybe lookup_swap_cache could return the page locked or return NULL, but then there's the same problem in the regular swapin that is async and the lock will go away when I/O completes, so there's still a window for the same race, so I thought it's not worth locking anything in lookup_swap_cache.

Quoted 3 lines

It can even happen when swap is near empty: through swapoff, or through reuse_swap_page(), at least.

Agreed, it can also happen with threads swapping in a not shared anon page.

Quoted 2 lines

I'm not aware of any bug we have from that, but sure, it comes as a surprise when you realize it.

:)

Quoted 10 lines

Quoted 4 lines

It's also possible to fix it by forcing do_wp_page to run but for a little while it won't be possible to rmap the the instantiated page so I didn't change that even if it probably would make life easier to memcg swapin handlers (maybe, dunno).

That's an interesting idea. I'm not clear what you have in mind there, but if we could get rid of ksm_does_need_to_copy(), letting do_wp_page() do the copy instead, that would be very satisfying. However, I suspect it would rather involve tricking do_wp_page() into doing it, involve a number of hard-to-maintain hacks, appealing to me but to nobody else!

It's actually next to trivial to make that change and it eliminates a dozen lines of code. I didn't to just make a strict fix that doesn't alter any logic of the code. But it's just enough to do:

if (ksm_might_need_to_copy())
flags |= FAULT_FLAG_WRITE;

Then the whole ksm_does_need_to_copy and swapcache variable and the rest of the code I added goes away (only the pageswapcache check after page_lock remains for the bug discussed at the top). pte_same in do_wp_page and do_swap_page then become both reliable because do_swap_page locks the swapcache before changing the pte to point to the page, and do_wp_page pins the old page before doing pte_same. So there's no risk of swap entry reuse that way and it eliminates some code.

Only downside of ksm_does_need_to_copy removal: rmappability of the swapcache for a ksm-swapin that isn't linear becomes impossible for a little window of time, but only split_huge_page requires rmappability always (only transparent hugepages are required to be precisely rmapped at all times if they are ever established in any pmd_trans_huge pmd), and migrate only requires what is remappable to remain remappable or remove_migration_pte will then crash (but it's ok if stuff isn't remappable before it's established as the page count won't match and migrate will temporarily abort). So there is no real problem if some swapcache (not transparent hugepage) established in a _new_ pte temporarily isn't remappable. OTOH reducing the window for lack of rmap is nice too considering it's not going to make any difference to do_swap_page.

The only one that really may benefit from ksm_does_need_to_copy that I can imagine memcg, I've no clue how memcg will feel when mem_cgroup_commit_charge_swapin aren't getting a swapcache. There are some checks for pageswapcache and it probably doesn't choke but I can't tell if some stat will go off by one or similar, or if it already works fine as is.