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 5F525C48BF6 for ; Thu, 7 Mar 2024 04:39:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B18606B00FE; Wed, 6 Mar 2024 23:39:13 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id AC86F6B00FF; Wed, 6 Mar 2024 23:39:13 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 990626B0100; Wed, 6 Mar 2024 23:39:13 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 842AA6B00FE for ; Wed, 6 Mar 2024 23:39:13 -0500 (EST) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 4C91BA103E for ; Thu, 7 Mar 2024 04:39:13 +0000 (UTC) X-FDA: 81868988586.22.34D9404 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.18]) by imf13.hostedemail.com (Postfix) with ESMTP id F2B552000B for ; Thu, 7 Mar 2024 04:39:09 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=EFWXapnA; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf13.hostedemail.com: domain of ying.huang@intel.com designates 192.198.163.18 as permitted sender) smtp.mailfrom=ying.huang@intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1709786351; 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=3wMdn6rvSt2TK5FsKTMnd/VkNRDUy6iH0YxXo6VogIU=; b=vZQlvA1uJwhljW2XlZh5anpJIuznJI80oge+VOGoTnZXZR0jTY4cTp2I1TDHNjgKV5SapG wpn4F6Vfcb08naFr+ry5eyS+/O7LVYfQE6aTHP1kfalLHVfMOboJnq7wE/xF55dC3XEtfr PcaoWC1jTQI8DX0InaHWlxXvMDTdczs= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=EFWXapnA; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf13.hostedemail.com: domain of ying.huang@intel.com designates 192.198.163.18 as permitted sender) smtp.mailfrom=ying.huang@intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1709786351; a=rsa-sha256; cv=none; b=zZkm4ApGJEhGa/FxGlhLBj1wSKWCqXcawaUpcfxbLgga79F8RQFOPBRPqq7mOpMJhPb4W8 us60IcQT9k6eFriEQUo29vFCkZF4kzE5X8qScE6c0M6K0F0I67B9aAUu6oRGlzCCNbcW7M v318yxALjIRRN+/b4sypxo9mKGQ9mNg= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1709786350; x=1741322350; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version; bh=d23Iig9hJmHScc/rMODy/liRKbgEi8K7tGamUjUBsGE=; b=EFWXapnA8YIq0GPr66b7kA/3DmsaHfoKXLJHGmuHFYuVcr2b3jBSt6cf 3CaAxFYYTTlPI+/tKMJEHluqUOxdq0LwzKNxLcVe+lPLodfd0XU/pXkcv CI6p8Zy6VyIVKl8bxgBt4KnUqyEr+5Vyus90y4wsY34LLrZcw8PHYjC/M /TjGmwKaItSstUyfKQOa0Qo1JyvCdrLse3HDbrKBOH75WZSAg1qPqKjli 0XSnoRGFQYeM2OJde/JHpVYIx8ZSkXWpMAMRip100Mi49VIXGQSC/X8nz 70o/22Qdl4dEW29jjhDM3jV5iuGnykQ4AEQB0txD90bfL0zGYD/53JssP A==; X-IronPort-AV: E=McAfee;i="6600,9927,11005"; a="4285652" X-IronPort-AV: E=Sophos;i="6.06,209,1705392000"; d="scan'208";a="4285652" Received: from orviesa006.jf.intel.com ([10.64.159.146]) by fmvoesa112.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Mar 2024 20:39:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.06,209,1705392000"; d="scan'208";a="10410969" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by orviesa006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Mar 2024 20:39:06 -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: (Ryan Roberts's message of "Wed, 6 Mar 2024 09:31:01 +0000") References: <20240305151349.3781428-1-ryan.roberts@arm.com> <875xy0842q.fsf@yhuang6-desk2.ccr.corp.intel.com> Date: Thu, 07 Mar 2024 12:37:10 +0800 Message-ID: <87plw67j49.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii X-Rspamd-Queue-Id: F2B552000B X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: 51fgutninhhkxmomeo8mqsxpgk9mwxre X-HE-Tag: 1709786349-919870 X-HE-Meta: U2FsdGVkX18/tXEu2k3rfktUrUMIGV8sygmZJfgMXtGZcRlwl0QQi1+sF/2igCQ07Tsf0zJm3FHjGBDK/0IKIfFQ9OYgWt6jLAAx7Bn6soElwJvIfv9U5sn2mH0RuA/rJAVPjGSHJmEJ1ln73/1vjb6FgTiiCsP0dapcJCKLc1+mlW/wGvq3GEpGi762p4rVu9XGhPEcL5jYaJKFc1BLdqZhmzIIN5kviXmSgurLKg3fz1a00znMZ/Gn3KeOru6HiGdNhGmbxSNlV70kcBVUFTpbCs7kFJlc3pYqLRiF0Z6VuvYuwnOG8nYEJBSldPkf/RvIVCkL6s5MJTQZqr9waY6HjzPYA5Fh5zidGXYLw0D6Ucpg8peIDJF90ZPw0MdurBiryMKg3DiB69ydXPjaBoeFjm5tDKgG75pOuhC1dosjbcOCU7OGM4mktwKWixFbAhKMdvZ72yo4xHlzyCfaq0ejaRkDcVAs++ksyin7jx/x5GRXekC1lJ6JKJV46BRHS4w5j1T4zZ6w6YdJPVvqDmSsscbnIhaZQRAdGA6j6vy7lKXDDn3Gx+XtqjmWC1qB6rwdPibXUFqshXHoeH7M6jk7kqxyqa0CzhoqtEcbhbOPRXid7SeQnxzAIZqL/ozMd/RtKRdVkDgxzTFgEJKm7ISYWkKGhVCJgWtY6Y2ef/SbvHHNtuBEGnNxOPxs7XUtbtGP0BFQXghxvq6kZ0pGLZIcuassu30Fw/VQ8FOxnxulynLboq/ywkr8fDfzWGx8+sqCyP9mWTnFbxnG19rKQ5lzlPoZUoD36fVq9iLzCJXdZj9/J+CmbCSeuZuMUYeJ5bnTw+YKmKzbxdfXDwYzCoVj2t5CLq+7a65mMT4RcV39FDn5wx2bLh4n5fUexxFHZdFHnIBHVUg= 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 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. Yes, we don't hold PTL there. After checking the code again. I think that there may be race condition as above without PTL. But I may miss something, again. >> So synchronize_rcu (called by swapoff) will wait zap_pte_range to release the pte lock. So this >> theoretical problem can't happen. Or am I miss something? > > For Huang Ying's example, I agree this can't happen because try_to_unuse() will > be waiting for the PTL (see the reply I just sent). > >> >> CPU0 CPU1 >> ---- ---- >> >> zap_pte_range >> pte_offset_map_lock -- spin_lock is held. >> free_swap_and_cache >> __swap_entry_free >> /* swap count become 0 */ >> swapoff >> try_to_unuse >> filemap_get_folio >> folio_free_swap >> /* remove swap cache */ >> percpu_ref_kill(&p->users); >> swap_page_trans_huge_swapped >> pte_unmap_unlock -- spin_lock is released. >> synchronize_rcu(); --> Will wait pte_unmap_unlock to be called? > > Perhaps you can educate me here; I thought that synchronize_rcu() will only wait > for RCU critical sections to complete. The PTL is a spin lock, so why would > synchronize_rcu() wait for the PTL to become unlocked? Please take a look at the following link, https://www.kernel.org/doc/html/next/RCU/whatisRCU.html#rcu-read-lock " Note that anything that disables bottom halves, preemption, or interrupts also enters an RCU read-side critical section. Acquiring a spinlock also enters an RCU read-side critical sections, even for spinlocks that do not disable preemption, as is the case in kernels built with CONFIG_PREEMPT_RT=y. Sleeplocks do not enter RCU read-side critical sections. " -- Best Regards, Huang, Ying > >> /* free si->swap_map[] */ >> >> Thanks. >> >>