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 0FEF8C54E49 for ; Fri, 8 Mar 2024 00:58:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 646F26B02EB; Thu, 7 Mar 2024 19:58:50 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 5F79F6B02EC; Thu, 7 Mar 2024 19:58:50 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 497AE6B02ED; Thu, 7 Mar 2024 19:58:50 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 355566B02EB for ; Thu, 7 Mar 2024 19:58:50 -0500 (EST) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id EBC5C1411E8 for ; Fri, 8 Mar 2024 00:58:49 +0000 (UTC) X-FDA: 81872061978.21.269E922 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.11]) by imf12.hostedemail.com (Postfix) with ESMTP id B252340005 for ; Fri, 8 Mar 2024 00:58:46 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=mRxymGJf; spf=pass (imf12.hostedemail.com: domain of ying.huang@intel.com designates 198.175.65.11 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1709859528; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Wla91iK2gk+6//1o1AiSyCfBLkHBdYAiSaek5lerYZ8=; b=g5osa174sFxgMgQd0oyUvx9yfy1pW02/0XvEXvEyim8D5ovZDdlIKvI36A1rGMBMS/pJBn KIRxhAgVNKvaet0smqzEOgB3zj7Ff6lNLtcGBQgoi3lT7U3xA9D8F5m4CqC37UYue/mQcR D7tuDaHp1cFPJ9eqQ21e7JK1h5AWz/Q= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1709859528; a=rsa-sha256; cv=none; b=jkIb/bKod03adhiuH3bFyzalvAJafEWpO7QE8yD2GxP6tqIUBTjUR4vkfYAwOmnWsndveS FhEl8kbGyx8YfGXWxP9Z0vsrRaQOVl0qaCd42bSwhF54wENx1RhYnUH8lNhJll/7YYqRMe LoB798qk7pEtgPmAPE7Wj7FrEeSwJkQ= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=mRxymGJf; spf=pass (imf12.hostedemail.com: domain of ying.huang@intel.com designates 198.175.65.11 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1709859528; x=1741395528; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version; bh=UnUW87fuiaXOWWLX9//SnpFXWJlC5GpqiAqCizoHHQ0=; b=mRxymGJfc61iUDskoMVf8qzEDbwwpns2YDaOc+kQVNk4zDlbbLdbhUuB b63hjKsT3ZH3GT1d8aJWgO/61rS7RZcvEX8o0UMqL0W2VCSJfRcMj6PmO mnRp5HIWVorporOabqZ5mX6edm4y+iYJxHr8B9oyYBE6SYVG0q+Ni2tR3 vU4B4zWO1zi0UKYddcEKCcgimvyZH/LalRyxBQ4lLnGdC0iLTpLIXjfLp 18LzBWo/reUbWtyZbyrBiWME1/8LL5ZnwkCsfU1ZKs41nF0+0ZEaL++cL pZaCaMN95QbHUNIoFmf3fE52CpFXdeZqnrOry3PfeS21fHwL8NrmJ4wbQ w==; X-IronPort-AV: E=McAfee;i="6600,9927,11006"; a="15133562" X-IronPort-AV: E=Sophos;i="6.07,108,1708416000"; d="scan'208";a="15133562" Received: from fmviesa005.fm.intel.com ([10.60.135.145]) by orvoesa103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Mar 2024 16:57:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.07,108,1708416000"; d="scan'208";a="14793930" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by fmviesa005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Mar 2024 16:57:12 -0800 From: "Huang, Ying" To: Ryan Roberts Cc: Miaohe Lin , Andrew Morton , David Hildenbrand , , , Subject: Re: [PATCH v1] mm: swap: Fix race between free_swap_and_cache() and swapoff() In-Reply-To: <29335a89-b14b-4ef3-abf8-0b41e6d0ec67@arm.com> (Ryan Roberts's message of "Thu, 7 Mar 2024 09:19:20 +0000") 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> <92672c62-47d8-44ff-bd05-951c813c95a5@arm.com> <87y1au5smu.fsf@yhuang6-desk2.ccr.corp.intel.com> <29335a89-b14b-4ef3-abf8-0b41e6d0ec67@arm.com> User-Agent: Gnus/5.13 (Gnus v5.13) Date: Fri, 08 Mar 2024 08:55:18 +0800 Message-ID: <87jzmd5yq1.fsf@yhuang6-desk2.ccr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ascii X-Rspamd-Queue-Id: B252340005 X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: 6drzujoqwbfsg7hzkohgbtgee9c7fum9 X-HE-Tag: 1709859526-883778 X-HE-Meta: U2FsdGVkX19wTBzIVQRxQt9nVlAGSjHaI6b5OKzCKDNBY78L+R6qF2yTPNThDq5TDEYC6I3Bf0opKGLcvJnR5HO+7wxFeIOJhDTbeL8b3fSE1tM4CumVaiFzgfl2BAHFm2wD7xRChuiuGv2dMgT7dmZ+E3C5FRE2EHZZExe7w77jlbyqi7Zr9nDHTRdmEyPt6UYOtu386fsyVljuALJIwdIp6X1BBiZ7o9Jm5beWayLaNglEz8SM9m4wD3+U9R9fA2Npej+qwcqWKp8QLLIKQ58YKy5BCAbGAxpQ+9SYKwJXX0YHyOEgiEs3KwZfeeRAYJQjGareMWDM/3nBF0Vlja7eMlV1AwO1raCMsxEocACVZOLHV0XMjWoWY6NfxbjOUMg9RtVm8WxYLGCvaVoz3/S+lz7xT75uJ4OZfIag7/y6LwyP3YaCE3R1b17fay2ygOZ52kFQnDzJLBFpanGNL9d1gNjrwQJDRS0+W+099xXv6YjFPBWDxNhVqKrmgpl3WgLDyvVNoK24ALxHy7Qrf3Fq1LYKieVXejSSSYPPhPVqbhKPNWc8xHAVyT+lfG7k/Z+2Ks9bVSiBnzY9JpwaemhXu/h1ETZwzxRO429KuJJNxFTx4W/BgYcRCQ3Z+Bb9auii7g+fKtJwIbdP7+hws1bTWi0/czTwYIpLfgEL3poJP8S5qosF2LfV5mJSBQV/zox6RmgwB5MRVAk+sHAS3fQ/l7CSEqUczzsQ3OIX7vKAl2xMi2eXRuR/cnS0eDucQk7T0mmZFkkJNllbEYCKrM2RhG2JrUpGAgz/L6pOIFHZvpkb5n6QtQsJCqHhjFX/0R002sTPYNQu9lostCNzSFFD9/Oze+QAr53XeeRqiSNf8E3Uer38v0bGN1Mgj9rsjiDCu2iNLEQ= 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: Ryan Roberts writes: > On 07/03/2024 08:54, Huang, Ying wrote: >> Ryan Roberts writes: >> >>> On 07/03/2024 07: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. >>> >>> Yes that's the shmem problem I've been trying to convey. Perhaps there are other >>> (extremely subtle) mechanisms that make this impossible, I don't know. >>> >>> Either way, given the length of this discussion, and the subtleties in the >>> syncrhonization mechanisms that have so far been identified, I think the safest >>> thing to do is just apply the patch. Then we have explicit syncrhonization that >>> we can trivially reason about. >> >> Yes. This is tricky and we can improve it. So I suggest to, >> >> - Revise the patch description to use shmem race as example except >> someone found it's impossible. >> >> - Revise the comments of get_swap_device() about RCU reader side lock >> (including IRQ off, spinlock, etc.) can prevent swapoff via >> synchronize_rcu() in swapoff(). >> >> - Revise the comments of synchronize_rcu() in swapoff(), which can >> prevent swapoff in parallel with RCU reader side lock including swap >> cache operations, etc. > > The only problem with this is that Andrew has already put my v2 into mm-*stable* :-| > > So (1) from that list isn't possible. I could do a patch for (2) and (3), but to > be honest, I think you would do a better job of writing it up than I would - any > chance you could post the patch? > Sure. I will do that. -- Best Regards, Huang, Ying