Received-SPF: pass (google.com: domain of linux-kernel+bounces-94990-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: "Huang, Ying" <ying.huang@intel.com>
To: Miaohe Lin <linmiaohe@huawei.com>
Cc: Ryan Roberts <ryan.roberts@arm.com>,  Andrew Morton
 <akpm@linux-foundation.org>,  David Hildenbrand <david@redhat.com>,
  <linux-mm@kvack.org>,  <linux-kernel@vger.kernel.org>,
  <stable@vger.kernel.org>
Subject: Re: [PATCH v1] mm: swap: Fix race between free_swap_and_cache() and
 swapoff()
In-Reply-To: <ff6aec00-f939-b7ba-c127-b133c4d95ee5@huawei.com> (Miaohe Lin's
	message of "Thu, 7 Mar 2024 10:38:47 +0800")
References: <20240305151349.3781428-1-ryan.roberts@arm.com>
	<875xy0842q.fsf@yhuang6-desk2.ccr.corp.intel.com>
	<c8fe62d0-78b8-527a-5bef-ee663ccdc37a@huawei.com>
	<af11bbca-3f6a-4db5-916c-b0d5b942352b@arm.com>
	<ff6aec00-f939-b7ba-c127-b133c4d95ee5@huawei.com>
Date: Thu, 07 Mar 2024 13:56:42 +0800
Message-ID: <87bk7q7ffp.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=ascii

Miaohe Lin <linmiaohe@huawei.com> 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@arm.com> 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?

--
Best Regards,
Huang, Ying

>> 
>>> So synchronize_rcu (called by swapoff) will wait zap_pte_range to release the pte lock. So this
>>> theoretical problem can't happen. Or am I miss something?
>> 
>> For Huang Ying's example, I agree this can't happen because try_to_unuse() will
>> be waiting for the PTL (see the reply I just sent).
>
> Do you mean the below message?
> "
> I don't think si->inuse_pages is decremented until __try_to_reclaim_swap() is
> called (per David, above), which is called after swap_page_trans_huge_swapped()
> has executed. So in CPU1, try_to_unuse() wouldn't see si->inuse_pages being zero
> until after CPU0 has completed accessing si->swap_map, so if swapoff starts
> where you have put it, it would get stalled waiting for the PTL which CPU0 has.
> "
>
> I agree try_to_unuse() will wait for si->inuse_pages being zero. But why will it waits
> for the PTL? It seems PTL is not used to protect si->inuse_pages. Or am I miss something?
>
>> 
>>>
>>> CPU0                                     CPU1
>>> ----                                     ----
>>>
>>> zap_pte_range
>>>   pte_offset_map_lock -- spin_lock is held.
>>>   free_swap_and_cache
>>>    __swap_entry_free
>>>    /* swap count become 0 */
>>>                                          swapoff
>>>                                            try_to_unuse
>>>                                              filemap_get_folio
>>>                                              folio_free_swap
>>>                                              /* remove swap cache */
>>> 					    percpu_ref_kill(&p->users);
>>>    swap_page_trans_huge_swapped
>>>   pte_unmap_unlock -- spin_lock is released.
>>> 					    synchronize_rcu();  --> Will wait pte_unmap_unlock to be called?
>> 
>> Perhaps you can educate me here; I thought that synchronize_rcu() will only wait
>> for RCU critical sections to complete. The PTL is a spin lock, so why would
>> synchronize_rcu() wait for the PTL to become unlocked?
>
> I assume PTL will always disable preemption which disables a grace period until PTL is released.
> But this might be fragile and I'm not really sure. I might be wrong.
>
> Thanks.
>> 
>> 
>>>                                            /* free si->swap_map[] */
>>>
>>> Thanks.
>>>
>>>
>> 
>> .
>>