[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1d27e93b-1c6b-4909-859f-e0756974a640@arm.com>
Date: Mon, 11 Mar 2024 09:44:47 +0000
From: Ryan Roberts <ryan.roberts@....com>
To: Huang Ying <ying.huang@...el.com>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
David Hildenbrand <david@...hat.com>, stable@...r.kernel.org
Subject: Re: [PATCH -v3] mm: swap: fix race between free_swap_and_cache() and
swapoff()
On 11/03/2024 08:44, Huang Ying wrote:
> From: Ryan Roberts <ryan.roberts@....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.com/
> Signed-off-by: Ryan Roberts <ryan.roberts@....com>
> Signed-off-by: "Huang, Ying" <ying.huang@...el.com> [patch description]
> Cc: David Hildenbrand <david@...hat.com>
> Cc: <stable@...r.kernel.org>
> Signed-off-by: Andrew Morton <akpm@...ux-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 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.
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;
> }
Powered by blists - more mailing lists