From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4AEE7C48BF6 for ; Thu, 7 Mar 2024 08:50:20 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 59A7F6B0128; Thu, 7 Mar 2024 03:50:19 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 54A566B0129; Thu, 7 Mar 2024 03:50:19 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 411EC6B012A; Thu, 7 Mar 2024 03:50:19 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 2F32F6B0128 for ; Thu, 7 Mar 2024 03:50:19 -0500 (EST) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id A94B31210F5 for ; Thu, 7 Mar 2024 08:50:18 +0000 (UTC) X-FDA: 81869621316.23.FC799DE Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by imf26.hostedemail.com (Postfix) with ESMTP id 34170140010 for ; Thu, 7 Mar 2024 08:50:13 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf26.hostedemail.com: domain of linmiaohe@huawei.com designates 45.249.212.255 as permitted sender) smtp.mailfrom=linmiaohe@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1709801415; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YsL7a8Tdp24XQ9D11QkKbV9rHrtRyOEgG9HWDFkpDmk=; b=w6cEwmOIyWso7cP5JJgg/t9QcKSs3f3z+Q9kmGUngneCdhukACVE2guMa+99v4egFw0VIU dEhrDLdO5G0Taqy1NdBQ+N7NGSppWiAOYP7Jg1Gh0Q4VvieDv1bf59nDzp2olrBBmh8uLc qPN1UClMcOEaqUD+WII8iwlafWdBXKQ= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf26.hostedemail.com: domain of linmiaohe@huawei.com designates 45.249.212.255 as permitted sender) smtp.mailfrom=linmiaohe@huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1709801415; a=rsa-sha256; cv=none; b=8oWp2AZE03eXnAqiedb/IX4wwy6jtpYIqBGa15SLJdrE0OPAiL871cTYwFQIZfmKLWWcHE bQFyqGr7eH7oPhpIKLTrIcwcwqErxUg/ffsrpnL47KUNGiOrCv723mnWSL4t8S1fcMiE8Q Rjh/PeGWUQDERHdI/YlWd52DGrZ+qTc= Received: from mail.maildlp.com (unknown [172.19.88.105]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4Tr2y24YNtz1Q9P2; Thu, 7 Mar 2024 16:48:10 +0800 (CST) Received: from canpemm500002.china.huawei.com (unknown [7.192.104.244]) by mail.maildlp.com (Postfix) with ESMTPS id 895611404F6; Thu, 7 Mar 2024 16:50:09 +0800 (CST) Received: from [10.173.135.154] (10.173.135.154) by canpemm500002.china.huawei.com (7.192.104.244) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.35; Thu, 7 Mar 2024 16:50:09 +0800 Subject: Re: [PATCH v1] mm: swap: Fix race between free_swap_and_cache() and swapoff() To: "Huang, Ying" CC: Ryan Roberts , Andrew Morton , David Hildenbrand , , , References: <20240305151349.3781428-1-ryan.roberts@arm.com> <875xy0842q.fsf@yhuang6-desk2.ccr.corp.intel.com> <87bk7q7ffp.fsf@yhuang6-desk2.ccr.corp.intel.com> <0925807f-d226-7f08-51d1-ab771b1a6c24@huawei.com> <8734t27awd.fsf@yhuang6-desk2.ccr.corp.intel.com> From: Miaohe Lin Message-ID: <588b8b8f-979c-a4e5-a332-8b0f89421ecd@huawei.com> Date: Thu, 7 Mar 2024 16:50:08 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <8734t27awd.fsf@yhuang6-desk2.ccr.corp.intel.com> Content-Type: text/plain; charset="windows-1252" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.173.135.154] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To canpemm500002.china.huawei.com (7.192.104.244) X-Rspam-User: X-Stat-Signature: 6f7worzacx5btpkqnac9zddnrxd4pwq5 X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 34170140010 X-HE-Tag: 1709801413-450723 X-HE-Meta: U2FsdGVkX1/cIEZrTAiCcdkna7XUl9133fQyRpoK85EQ6PAWO9haPHg6szapOWnlAK6a3t/1QGXmRasS+7odrn9oEpDPtuqSD7/xalDLEGzRhGiGk93Ukov8gNb5RSgNp7nzJSYWpUDfWOsgRMBQ8H7NbEMfz4FS9+0lFfHFjUj4s8r9nI7xNWnvD5sL+ItJB7Huqo1WtzoI9vRjz+se66kf6CyCDBliCMp2HZqFJbdJQNVGDNj5srbXua7NJX6tCrqy02xNd4chxE+wrWNzvrCwAT8XARPwY9eIQlSGq1Lyt51Qw/i6h4SUY2EYnhaZ7kBcTUqW0NKteTVZA30lPNcjLUxzkoaY9WmRdx+MfaxEGiJxCwCP8EK2jis1lVy4lPdymFYixxhrR/X9esXI9R4/o/KpT/xqUx05qkA0Gvmwg9KOnlMkxDjavlGJBZ6G9n9dERaDqgNgwSmPYaGHzv/kMAO6PhStcQy7VUEmZqDkVG/ydcL0drFDwwPIQbjDdFzTtoshpJlpfwQLjDkrdOi2xoOFRpiWLDYQKvlNhRzSIoTSfIufNErV0Mza4J5tlZvzedYL9lWDjKuIMMUBo6xUNvmZOjtdcx94kJke4q9B55hcKk8oNjD+gxg95sgAZBjKQLbv+ekE9m3j16PiWL7ii20XGEAT4Ys0snE0+8XT/RjNL9HLwGTjD1q/dioNZLvcnc09l1NUR5G2Kwchohd7qTqLSfH6QgYQ10EJhO5Yiq+NT9ei8SVhQzgH9xNgaQxCmhEMyWl55tQYPMNlYdEXvPDQr24oytTSTbzg+IiN786hA8eC9FmRDOEna/o0p6rI9WJGgjFLEg0fBCvE5d60hfiqVVLr X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 2024/3/7 15:34, Huang, Ying wrote: > Miaohe Lin writes: > >> On 2024/3/7 13:56, Huang, Ying wrote: >>> Miaohe Lin 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 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 > . >