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 1BF66C48BF6 for ; Thu, 7 Mar 2024 09:19:28 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8DA3F6B0133; Thu, 7 Mar 2024 04:19:27 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 88C8F6B0134; Thu, 7 Mar 2024 04:19:27 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7522C6B0135; Thu, 7 Mar 2024 04:19:27 -0500 (EST) 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 663F86B0133 for ; Thu, 7 Mar 2024 04:19:27 -0500 (EST) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id D04D312053D for ; Thu, 7 Mar 2024 09:19:26 +0000 (UTC) X-FDA: 81869694732.19.410406E Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf05.hostedemail.com (Postfix) with ESMTP id E5D55100017 for ; Thu, 7 Mar 2024 09:19:24 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=none; spf=pass (imf05.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1709803165; 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=HuBU2eobQkyWJZeYadAxz/cAzjUfAmkYWs6jaO2YCNs=; b=DdfdXh4FqPcR6W6EYfOUbLCQLrWv0e2oWbMy+zgWtK45ZsZpaW8t30WnA0ybdpR/qVjwHb u1kxWcMH0RIkqzNeMAP32e6+uOTD6Ps86vttKd9WoxXhzgXRqw2kaG6BxOn0GRnYxHeJ4+ NtCZAIurK4bj0KHsACoAM0nz8SoYRpE= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1709803165; a=rsa-sha256; cv=none; b=PoNwIpnQYFOL0q25qtkTwTuKVANAtSOs3CHErSz02w/xSp/PiNeQVoF8wuMzCGxTPzjvVq 6rct4nzX5POdO7PR7Qou/L608Cc/b36iQWQMHsR96hiAWLP5wrMgikEuna6Cjn/xES8T7+ vZo6T98k9R7BokcguDpQweqB3+lWtq8= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=none; spf=pass (imf05.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com 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 03D381FB; Thu, 7 Mar 2024 01:20:01 -0800 (PST) Received: from [10.57.68.241] (unknown [10.57.68.241]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BC6B63F762; Thu, 7 Mar 2024 01:19:22 -0800 (PST) Message-ID: <29335a89-b14b-4ef3-abf8-0b41e6d0ec67@arm.com> Date: Thu, 7 Mar 2024 09:19:20 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1] mm: swap: Fix race between free_swap_and_cache() and swapoff() Content-Language: en-GB To: "Huang, Ying" Cc: Miaohe Lin , Andrew Morton , David Hildenbrand , linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org 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> From: Ryan Roberts In-Reply-To: <87y1au5smu.fsf@yhuang6-desk2.ccr.corp.intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: E5D55100017 X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: 7fc5pouhtb136fx8tcaoab63a9mpdowc X-HE-Tag: 1709803164-325947 X-HE-Meta: U2FsdGVkX1+ZDZuM2GLQqw3bEjIUJfO26ALLkFju+LKOjvCx5lxPdVO6xpQNrxIDmh64G7aPq1tN6FgxKmm7SRdW1maTTwXaLbw083dmjMI+eFUg1lKq/cgzLHp3yKjQwDJw06dUUgae3LNkzBBYmUAHQZ1s6xRJnjrel5P6mh1Yj7jnxq0iKqDw/Vl7ks0E3NcC5KRh07w2Pgdy/GDwzqVBLv4Ai+BCBQ0X3ijugWJbV0L89Qyy1iZ9ZkfChpsu1NtDqovCeIulW50gXVOCxRh8WQkaczlAexgG3K1FvEHpj6GIHCWqHSmbDUTyVBgU97q1dx98x2ErSyqvgNkxrS4HdsCxqWl6M3DQyWjCdnCXWNxGY680JNH1+0nS3xpkokwRpi3WHFdyG3GCGuL0jfGgvuR/m35cntOZA5HP4zSynJrVcdFC67N4yG5RxC5HFkAUPg2K0SgS8C6HDCpGzWoSHH0z9IWVKvN8zMN5K/0NXii3ETUIQ6x6HcBhdRnPwWl5MMOOYqLiIvWXjZAp8o4ZwsN6wVmCXNCuZNkpAI2zRP9D0NOfPzThhxCfvMwvMdqAmxgI2skbS9KPD+B8Sq1PdcWgU6AixxvEH7o5O99ARTaJpnvr4rXqxKYJ5fWnzzkJ89XdssNwEDj9xGLiOjpVfFbUdUWk8LiR5C/8DWkqJKVwifwXti7sJcBIBrjyGWybv3SMIbNIz4rVXk0YihcHfIsu+u39VfnHQ/BEYJfKldc0xg86cNrz0JAAFSCj8cspAZpWNU321/nyeYyEeDnDDXkPDelmc+puaEVFIBbPDH0CyIkonQ0xCQaqsXKwVjrUHxGhtZEP+Rz0es1Zv+ENvTN8bvZfwSL9QacAXRTJHujaamZFhDEAgjr+ds0X4Tzmwk4lzgRkbqil7VoqZxH3dee+py05msBcePVQduTJlUQILDu8KtRKu4P1/74wvGUfR6f5CyhoftyChaS kPsRPt5/ mioz51q2firGfairPux+4e0pRg/Wfed8PKB/ZyAB190ELN81ZD9sD0hZ6n6xQGt8j/SMKgUyZdE5zhskwjcXyJajjulu3uIiZET/93nmTzFoz1rZNSgu7QQws8pjFWkVl0W8YRoFQSb7c82dux1sR9J2lwSG6IdroU2ZC7zBdXuShQ9XlzsVzdUTa+8BS3cLjODeBHxyiw9YWKSYUxLIL6e45/rP5hMEWHW/T4miQOUTg0HQ80IQ4QXAAbS41rlpXoXA03qPohxFZFCnCsT8pWM8gDg== 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 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? > > -- > Best Regards, > Huang, Ying