[PATCH -v3] mm: swap: fix race between free_swap_and_cache() and swapoff()

[Huang Ying]

From: Ryan Roberts <ryan.roberts@xxxxxxx>

There was previously a theoretical window where swapoff() could run and
teardown a swap_info_struct while a call to free_swap_and_cache() was
running in another thread.  This could cause, amongst other bad
possibilities, swap_page_trans_huge_swapped() (called by
free_swap_and_cache()) to access the freed memory for swap_map.

This is a theoretical problem and I haven't been able to provoke it from a
test case.  But there has been agreement based on code review that this is
possible (see link below).

Fix it by using get_swap_device()/put_swap_device(), which will stall
swapoff().  There was an extra check in _swap_info_get() to confirm that
the swap entry was not free.  This isn't present in get_swap_device()
because it doesn't make sense in general due to the race between getting
the reference and swapoff.  So I've added an equivalent check directly in
free_swap_and_cache().

Details of how to provoke one possible issue:

--8<-----

CPU0                               CPU1
----                               ----
shmem_undo_range
  shmem_free_swap
    xa_cmpxchg_irq
    free_swap_and_cache
      __swap_entry_free
      /* swap_count() become 0 */
                                   swapoff
                                     try_to_unuse
                                       shmem_unuse /* cannot find swap entry */
                                       find_next_to_unuse
                                       filemap_get_folio
                                       folio_free_swap
                                       /* remove swap cache */
                                       /* free si->swap_map[] */
      swap_page_trans_huge_swapped <-- access freed si->swap_map !!!

--8<-----

Link: https://lkml.kernel.org/r/20240306140356.3974886-1-ryan.roberts@xxxxxxx
Closes: https://lore.kernel.org/linux-mm/8734t27awd.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
Signed-off-by: Ryan Roberts <ryan.roberts@xxxxxxx>
Signed-off-by: "Huang, Ying" <ying.huang@xxxxxxxxx> [patch description]
Cc: David Hildenbrand <david@xxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---
Hi, Andrew,

If it's not too late.  Please replace v2 of this patch in mm-stable
with this version.

Changes since v2:

 - Remove comments for get_swap_device() because it's not correct.
 - Revised patch description about the race condition description.

Changes since v1:

 - Added comments for get_swap_device() as suggested by David
 - Moved check that swap entry is not free from get_swap_device() to
   free_swap_and_cache() since there are some paths that legitimately call with
   a free offset.

Best Regards,
Huang, Ying

 mm/swapfile.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 2b3a2d85e350..9e0691276f5e 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1609,13 +1609,19 @@ int free_swap_and_cache(swp_entry_t entry)
 	if (non_swap_entry(entry))
 		return 1;
 
-	p = _swap_info_get(entry);
+	p = get_swap_device(entry);
 	if (p) {
+		if (WARN_ON(data_race(!p->swap_map[swp_offset(entry)]))) {
+			put_swap_device(p);
+			return 0;
+		}
+
 		count = __swap_entry_free(p, entry);
 		if (count == SWAP_HAS_CACHE &&
 		    !swap_page_trans_huge_swapped(p, entry))
 			__try_to_reclaim_swap(p, swp_offset(entry),
 					      TTRS_UNMAPPED | TTRS_FULL);
+		put_swap_device(p);
 	}
 	return p != NULL;
 }
-- 
2.39.2