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 AF7AEC5475B for ; Mon, 11 Mar 2024 09:44:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F237F6B007B; Mon, 11 Mar 2024 05:44:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id ED04D6B0081; Mon, 11 Mar 2024 05:44:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D98E56B0082; Mon, 11 Mar 2024 05:44:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id C78CC6B007B for ; Mon, 11 Mar 2024 05:44:54 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 94F521A12C6 for ; Mon, 11 Mar 2024 09:44:54 +0000 (UTC) X-FDA: 81884274108.13.B8A633B Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf01.hostedemail.com (Postfix) with ESMTP id 6E5F140005 for ; Mon, 11 Mar 2024 09:44:52 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf01.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1710150292; a=rsa-sha256; cv=none; b=kvUi/sl8IABK1h2N1fuy6pSTFgSiXNE1Ln3QF0Ow0OHK6T921t4tR0LMPZrj6auf8XVYYZ MrVd1NRshwZ2Q8vZoX5jRAdLP6ewVPqKIUG6TH09T0VzzcbuFV9emufBGGsSvtGzNcTUei +1OIqjy4GNbevDM57FpUNFhb0wBqB4M= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf01.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1710150292; 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=l41GMzG0VZNQeL2Fm/Gvray7kkO6nxLDk61wTJp266w=; b=Mfmjf9tuC7Albzau64ru9GuK4lYJWU2PI8P2H4t+mpImn4jyBz6/hR18AYEchPz9k1dQZb Dcw2MIw4mP/utY5L1J9DAQem1LUQsN3YWEn2HkAHozELI3fuIa0vJjBNzDRsCevMbeA+xH mEg3IVDMHq5bX19WRGtMYPZfL+7NnvM= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DD1CBFEC; Mon, 11 Mar 2024 02:45:27 -0700 (PDT) Received: from [10.57.68.246] (unknown [10.57.68.246]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E3CA03F762; Mon, 11 Mar 2024 02:44:49 -0700 (PDT) Message-ID: <1d27e93b-1c6b-4909-859f-e0756974a640@arm.com> Date: Mon, 11 Mar 2024 09:44:47 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH -v3] mm: swap: fix race between free_swap_and_cache() and swapoff() Content-Language: en-GB To: Huang Ying , Andrew Morton Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, David Hildenbrand , stable@vger.kernel.org References: <20240311084426.447164-1-ying.huang@intel.com> From: Ryan Roberts In-Reply-To: <20240311084426.447164-1-ying.huang@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 6E5F140005 X-Stat-Signature: dz5yhfq11yzc631i5d7ddt3ic8a35gw8 X-Rspam-User: X-HE-Tag: 1710150292-800102 X-HE-Meta: U2FsdGVkX19I044gXKOy1hIsyr7XrMMw2rWUSCQH0AbNq6zs6RshRYCSkah1u23NGYJvIavbWIAerVWvX9XBzSz9JrAZ7ixnD6JPCTCP7HTHMCseCM3hVXSl9OrYfWCFJ2RWcjZtzBa34jT5SOrfBUZfb56IqLg9V6yHhrj8hpdhY4m6FRCHLw+wIPliWaGZTe6Oi0S5629CyZd0ewiEprNie23TcYo5fUSlOw3ZIFpnN/+XdDdTSQs3+qMDM9FSu9uM4ivT7d1ZY5ckXDSFLvb0Shn2hnsKtKjmaOq3+kSFWHZfqjbp0jaq2b2cEORCfDy8ycKrD+Y0GfHYacLOqXrY08LX++rfP8v8JIGiS0RHouD3zb/rz7opVuK4JpIQ/vIXtUYrp+iGY6cC9McUuZJ5cPAnG/X6B+vkFdvCi2pPx4m7XG3bruVXfcgnyzDZjwTFb+HPxNiPc1euBIEorX0+vobKq2KUqkV0CMME703sxiCNPVkDNPfYOor1/HOpLBiNYXkY6wYXtqfR7en/aERdCvYSy5TbYRO0jS3lfBstp0uifuvsnQn4uJqCgQvonCcz2khDiV9clRDeQNOPq6Wu62ruBd71Dmbgh0SoufU6ZmJdvOwwnAuz/67p4vZJHlZ7952ONiED9sfhmUrVXZ+CG9EB7+pMTN9UxZVhUsyPqv/H6oMP3xNfeaJEhYTMR/6N3th70O9mLWSB/4JBXCMb8t5DrqHaQTEkapaVU1WWRf5ceA0Q5fCt6VeNEmCl01tEZm2Gylw9iGL3ctq8WshXdnB7tqapTZFhjFD5GQ4iT2Wf3zfOdoI7FmdkdIXxkSiYlkqPI/cUsgXyypdvgiJtSy7qlv/IRT2Hs/Mb7Taj7V8FWuq1GFR9+IxDv1hUsK2l4FZIizf7oxcu0z+yFGIek2dKnQhPhJ/2LWu9WJFZljXmGdkoagpu3+S5SyPut7It8zE9DoUhhI1Wy6z Cs2hKjS0 jI9UxSVCQOmDt0KD8PEKvbhBh71sIqWi69+rGrKHAEvdWnQrt6pdlSGBMTTxNlwcoWb6Kz7uBSR4Ic8coSgBH/TBFN0i2ZnPzRdApWBa3sqWPNcRZjCsi/YqAawxZ/toL8Iu3GWwjc+N4x9xsousRlLI1vT6msvpZD3FtGFc11ytsX32ijDNRChOy7uyCZjEtzoUEWLWCTNTL+wzUqFBbbEFXcUvrIkpUSPokANC6BDZqlXdeV9a1X3gmQpY7/MLYMdlCyyFsd+EEoFc/G6YApzPDKKoyvtopRe0OZamCTob4snvHqPsJnacVVSBeaSD92BsvYBvjVzXYGwsGTEIpSbUlH+FDN9rAIaI1binDESh+mHF6MCv4Z9S0SoTw1I5SQJ3k4NvZQy+AX5bEOXQTaFLeRjnUaDSxFKnh 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 11/03/2024 08:44, Huang Ying wrote: > From: Ryan Roberts > > 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 not free. This isn't present in get_swap_device() > because it doesn't make sense in general due to the race between getting > the reference and swapoff. So I've added an equivalent check directly in > free_swap_and_cache(). > > Details of how to provoke one possible issue: > > --8<----- > > 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 !!! > > --8<----- > > Link: https://lkml.kernel.org/r/20240306140356.3974886-1-ryan.roberts@arm.com > Closes: https://lore.kernel.org/linux-mm/8734t27awd.fsf@yhuang6-desk2.ccr.corp.intel.com/ > Signed-off-by: Ryan Roberts > Signed-off-by: "Huang, Ying" [patch description] > Cc: David Hildenbrand > Cc: > Signed-off-by: Andrew Morton > --- > Hi, Andrew, > > If it's not too late. Please replace v2 of this patch in mm-stable > with this version. Thanks for sorting this out, Huang, Ying! I saw your note asking if I could do it, and it was on my list, but I've been busy debugging other urgent issues in mm-stable. That should be solved now so unblocks me finishing the testing on my large folios swap-out v4 series. Hopefully that will be incomming in the next couple of days. You did previously suggest you wanted some comments around synchronise_rcu() in swapoff(), but I don't see those here. I don't think that should hold this up though. Thanks, Ryan > > Changes since v2: > > - Remove comments for get_swap_device() because it's not correct. > - Revised patch description about the race condition description. > > Changes since v1: > > - Added comments for get_swap_device() as suggested by David > - Moved check that swap entry is not free from get_swap_device() to > free_swap_and_cache() since there are some paths that legitimately call with > a free offset. > > Best Regards, > Huang, Ying > > mm/swapfile.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 2b3a2d85e350..9e0691276f5e 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -1609,13 +1609,19 @@ int free_swap_and_cache(swp_entry_t entry) > if (non_swap_entry(entry)) > return 1; > > - p = _swap_info_get(entry); > + p = get_swap_device(entry); > if (p) { > + if (WARN_ON(data_race(!p->swap_map[swp_offset(entry)]))) { > + put_swap_device(p); > + return 0; > + } > + > count = __swap_entry_free(p, entry); > if (count == SWAP_HAS_CACHE && > !swap_page_trans_huge_swapped(p, entry)) > __try_to_reclaim_swap(p, swp_offset(entry), > TTRS_UNMAPPED | TTRS_FULL); > + put_swap_device(p); > } > return p != NULL; > }