Received-SPF: pass (google.com: domain of linux-kernel+bounces-95025-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Subject: Re: [PATCH v1] mm: swap: Fix race between free_swap_and_cache() and
 swapoff()
To: "Huang, Ying" <ying.huang@intel.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>
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>
 <87bk7q7ffp.fsf@yhuang6-desk2.ccr.corp.intel.com>
From: Miaohe Lin <linmiaohe@huawei.com>
Message-ID: <0925807f-d226-7f08-51d1-ab771b1a6c24@huawei.com>
Date: Thu, 7 Mar 2024 14:50:10 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <87bk7q7ffp.fsf@yhuang6-desk2.ccr.corp.intel.com>
Content-Type: text/plain; charset="windows-1252"
Content-Language: en-US
Content-Transfer-Encoding: 7bit

On 2024/3/7 13:56, Huang, Ying wrote:
> 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?

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?

Thanks.

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