Re: [PATCH v1] mm: swap: Fix race between free_swap_and_cache() and swapoff()

[Miaohe Lin]

On 2024/3/7 15:34, Huang, Ying wrote:
> Miaohe Lin <linmiaohe@xxxxxxxxxx> writes:
> 
>> On 2024/3/7 13:56, Huang, Ying wrote:
>>> Miaohe Lin <linmiaohe@xxxxxxxxxx> writes:
>>>
>>>> On 2024/3/6 17:31, Ryan Roberts wrote:
>>>>> On 06/03/2024 08:51, Miaohe Lin wrote:
>>>>>> On 2024/3/6 10:52, Huang, Ying wrote:
>>>>>>> Ryan Roberts <ryan.roberts@xxxxxxx> writes:
>>>>>>>
>>>>>>>> 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 valid. This wasn't present in get_swap_device() so
>>>>>>>> I've added it. I couldn't find any existing get_swap_device() call sites
>>>>>>>> where this extra check would cause any false alarms.
>>>>>>>>
>>>>>>>> Details of how to provoke one possible issue (thanks to David Hilenbrand
>>>>>>>> for deriving this):
>>>>>>>>
>>>>>>>> --8<-----
>>>>>>>>
>>>>>>>> __swap_entry_free() might be the last user and result in
>>>>>>>> "count == SWAP_HAS_CACHE".
>>>>>>>>
>>>>>>>> swapoff->try_to_unuse() will stop as soon as soon as si->inuse_pages==0.
>>>>>>>>
>>>>>>>> So the question is: could someone reclaim the folio and turn
>>>>>>>> si->inuse_pages==0, before we completed swap_page_trans_huge_swapped().
>>>>>>>>
>>>>>>>> Imagine the following: 2 MiB folio in the swapcache. Only 2 subpages are
>>>>>>>> still references by swap entries.
>>>>>>>>
>>>>>>>> Process 1 still references subpage 0 via swap entry.
>>>>>>>> Process 2 still references subpage 1 via swap entry.
>>>>>>>>
>>>>>>>> Process 1 quits. Calls free_swap_and_cache().
>>>>>>>> -> count == SWAP_HAS_CACHE
>>>>>>>> [then, preempted in the hypervisor etc.]
>>>>>>>>
>>>>>>>> Process 2 quits. Calls free_swap_and_cache().
>>>>>>>> -> count == SWAP_HAS_CACHE
>>>>>>>>
>>>>>>>> Process 2 goes ahead, passes swap_page_trans_huge_swapped(), and calls
>>>>>>>> __try_to_reclaim_swap().
>>>>>>>>
>>>>>>>> __try_to_reclaim_swap()->folio_free_swap()->delete_from_swap_cache()->
>>>>>>>> put_swap_folio()->free_swap_slot()->swapcache_free_entries()->
>>>>>>>> swap_entry_free()->swap_range_free()->
>>>>>>>> ...
>>>>>>>> WRITE_ONCE(si->inuse_pages, si->inuse_pages - nr_entries);
>>>>>>>>
>>>>>>>> What stops swapoff to succeed after process 2 reclaimed the swap cache
>>>>>>>> but before process1 finished its call to swap_page_trans_huge_swapped()?
>>>>>>>>
>>>>>>>> --8<-----
>>>>>>>
>>>>>>> I think that this can be simplified.  Even for a 4K folio, this could
>>>>>>> happen.
>>>>>>>
>>>>>>> CPU0                                     CPU1
>>>>>>> ----                                     ----
>>>>>>>
>>>>>>> zap_pte_range
>>>>>>>   free_swap_and_cache
>>>>>>>   __swap_entry_free
>>>>>>>   /* swap count become 0 */
>>>>>>>                                          swapoff
>>>>>>>                                            try_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 !!!
>>>>>>
>>>>>> Sorry for jumping the discussion here. IMHO, free_swap_and_cache is called with pte lock held.
>>>>>
>>>>> I don't beleive it has the PTL when called by shmem.
>>>>
>>>> In the case of shmem, folio_lock is used to guard against the race.
>>>
>>> I don't find folio is lock for shmem.  find_lock_entries() will only
>>> lock the folio if (!xa_is_value()), that is, not swap entry.  Can you
>>> point out where the folio is locked for shmem?
>>
>> You're right, folio is locked if not swap entry. That's my mistake. But it seems above race is still nonexistent.
>> shmem_unuse() will first be called to read all the shared memory data that resides in the swap device back into
>> memory when doing swapoff. In that case, all the swapped pages are moved to page cache thus there won't be any
>> xa_is_value(folio) cases when calling shmem_undo_range(). free_swap_and_cache() even won't be called from
>> shmem_undo_range() after shmem_unuse(). Or am I miss something?
> 
> I think the following situation is possible.  Right?
> 
> 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 !!!
> 
> shmem_undo_range can run earlier.

Considering above case, I tend to agree it's possible. I can't figure out a mechanism to make it impossible yet.

Thanks.

> 
> --
> Best Regards,
> Huang, Ying
> .
>