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()
Date: Thu, 07 Mar 2024 15:34:42 +0800 [thread overview]
Message-ID: <8734t27awd.fsf@yhuang6-desk2.ccr.corp.intel.com> (raw)
In-Reply-To: <0925807f-d226-7f08-51d1-ab771b1a6c24@huawei.com> (Miaohe Lin's message of "Thu, 7 Mar 2024 14:50:10 +0800")
Miaohe Lin <linmiaohe@huawei.com> writes:
> 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?
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.
--
Best Regards,
Huang, Ying
next prev parent reply other threads:[~2024-03-07 7:36 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-05 15:13 [PATCH v1] mm: swap: Fix race between free_swap_and_cache() and swapoff() Ryan Roberts
2024-03-05 15:50 ` David Hildenbrand
2024-03-05 16:33 ` Ryan Roberts
2024-03-05 22:05 ` David Hildenbrand
2024-03-06 2:39 ` Huang, Ying
2024-03-06 8:10 ` Ryan Roberts
2024-03-06 2:19 ` Johannes Weiner
2024-03-06 8:23 ` Ryan Roberts
2024-03-06 2:52 ` Huang, Ying
2024-03-06 8:51 ` Miaohe Lin
2024-03-06 9:31 ` Ryan Roberts
2024-03-07 2:38 ` Miaohe Lin
2024-03-07 5:56 ` Huang, Ying
2024-03-07 6:50 ` Miaohe Lin
2024-03-07 7:34 ` Huang, Ying [this message]
2024-03-07 7:48 ` Ryan Roberts
2024-03-07 8:54 ` Huang, Ying
2024-03-07 9:19 ` Ryan Roberts
2024-03-08 0:55 ` Huang, Ying
2024-03-07 8:50 ` Miaohe Lin
2024-03-07 4:37 ` Huang, Ying
2024-03-07 4:39 ` Huang, Ying
2024-03-06 9:03 ` Ryan Roberts
2024-03-07 5:48 ` Huang, Ying
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8734t27awd.fsf@yhuang6-desk2.ccr.corp.intel.com \
--to=ying.huang@intel.com \
--cc=akpm@linux-foundation.org \
--cc=david@redhat.com \
--cc=linmiaohe@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=ryan.roberts@arm.com \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).