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 > . >
