Ryan Roberts ryan.roberts@arm.com writes:

On 11/03/2024 08:44, Huang Ying wrote:

...

From: Ryan Roberts ryan.roberts@arm.com

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@arm.com Closes: https://lore.kernel.org/linux-mm/8734t27awd.fsf@yhuang6-desk2.ccr.corp.intel... Signed-off-by: Ryan Roberts ryan.roberts@arm.com Signed-off-by: "Huang, Ying" ying.huang@intel.com [patch description] Cc: David Hildenbrand david@redhat.com Cc: stable@vger.kernel.org Signed-off-by: Andrew Morton akpm@linux-foundation.org

---

Hi, Andrew,

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

Thanks for sorting this out, Huang, Ying! I saw your note asking if I could do it, and it was on my list, but I've been busy debugging other urgent issues in mm-stable. That should be solved now so unblocks me finishing the testing on my large folios swap-out v4 series. Hopefully that will be incomming in the next couple of days.

You are welcome!

You did previously suggest you wanted some comments around synchronise_rcu() in swapoff(), but I don't see those here. I don't think that should hold this up though.

I will send a separate patch for that, including comments for get_swap_device() and around synchronize_rcu() in swapoff().

-- Best Regards, Huang, Ying

Thanks, Ryan

...

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;

}