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 08186C4829E for ; Sun, 18 Feb 2024 08:02:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2551D6B008A; Sun, 18 Feb 2024 03:02:03 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 205A26B008C; Sun, 18 Feb 2024 03:02:03 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0A6AF6B0092; Sun, 18 Feb 2024 03:02:03 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id E74B96B008A for ; Sun, 18 Feb 2024 03:02:02 -0500 (EST) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 8F5BD1C0044 for ; Sun, 18 Feb 2024 08:02:02 +0000 (UTC) X-FDA: 81804181284.01.FF00D47 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.18]) by imf11.hostedemail.com (Postfix) with ESMTP id 1BBA240012 for ; Sun, 18 Feb 2024 08:01:59 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=YFbDkigH; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf11.hostedemail.com: domain of ying.huang@intel.com designates 198.175.65.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=1708243320; 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=BuOTEGEtjLJqCsT6jfHryYJIZYXsoLW7XYh8J8ojPcg=; b=CDQ7vLM3AzRb3XEgmL5Ipm4HWJ9OZuYyQGcX8XdH7mSBKxMic2Gmd2roYoPR9t8oPmJfYm Skts9T23PK58PbSpSXUSmwbY1+bEVJ4y1fCaCFWC0pcGSYtp0m+w6J2m5rXHYH4hGHVeeQ dHAQ1JMlee5fKXiNnWaS7V9mKwBgIcw= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=YFbDkigH; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf11.hostedemail.com: domain of ying.huang@intel.com designates 198.175.65.18 as permitted sender) smtp.mailfrom=ying.huang@intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708243320; a=rsa-sha256; cv=none; b=uKAZv0JWc4CHO9rUTYphhJKLjxB6iBaGE04LHQZTZbHOMN27FyEm17jtD0QodiWAJ1W8EE LdxI1ru+7DUgQve9qT5bY9IHywEde0omBkGnSoO03R869Ihpbb6Nt6wNOnf5XdJOOKORzw 5IdFEZssvFMvNTdgN5a1/bximqbVetU= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1708243320; x=1739779320; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version; bh=lqXIwndud1ddMp3xILkmsTnX7OTQWZCxIFPe2f0/ln0=; b=YFbDkigHzOMzlv/li1onNzNwSchpYFfzxO23hIsYJAL7uCc/C0SWmeP6 Aom2UU3fGHWuFAVnseyLCnlxBCPZmZoUhhCdRlTWIt0AuyjbOBn1Jdnrw EbjT/PnpIF5S710IvKaSgzYJMhqS3CEYWjk5is6ujPpnvro1tt/lSHWIV /XjQzOYzpyQSOrf3Bu63tVEzc8GR2w8hqB1npypjqO3JyY46M7Z+V/Pgn rnmsdd8pWvpu86TDAA/xpddhBzEPDmN9Md+JnUlwQDEMpDndHPOA3Vqs2 aN8XwrNcbLo1zbOzZ5LiICiGrVKvssOFD4Q3w3YpbRt87Quekhw3zyz+e w==; X-IronPort-AV: E=McAfee;i="6600,9927,10987"; a="2445759" X-IronPort-AV: E=Sophos;i="6.06,168,1705392000"; d="scan'208";a="2445759" Received: from fmviesa008.fm.intel.com ([10.60.135.148]) by orvoesa110.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Feb 2024 00:01:58 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.06,168,1705392000"; d="scan'208";a="4364943" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by fmviesa008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Feb 2024 00:01:54 -0800 From: "Huang, Ying" To: David Hildenbrand Cc: Kairui Song , linux-mm@kvack.org, Andrew Morton , Chris Li , Minchan Kim , Yu Zhao , Barry Song , SeongJae Park , Hugh Dickins , Johannes Weiner , Matthew Wilcox , Michal Hocko , Yosry Ahmed , stable@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] mm/swap: fix race when skipping swapcache In-Reply-To: (David Hildenbrand's message of "Fri, 16 Feb 2024 17:53:25 +0100") References: <20240216095105.14502-1-ryncsn@gmail.com> Date: Sun, 18 Feb 2024 15:59:59 +0800 Message-ID: <871q9atd6o.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: 1BBA240012 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: 84tj9e4p59b4hywwnrad43g1wyotouxf X-HE-Tag: 1708243319-222593 X-HE-Meta: U2FsdGVkX18+56TmNevG1v0UAUn8hK0T766A42yyr/ScQCWLStyDeSXylwnXAhL6ZIX//WSwjTcrxYkGNP3Z24+ptDhNKx3ZVzlNdxU9VECiupl+v77SDvWHRy5t1svXOj+DNlEPTA/CCQDlHoYI5EK46tacy4TEnWUFtAsbr9azBELq3e/CrrvlkJIFWrm9Q0uQPDBk7jW32KtJQ8gQVKM8cXPYIRfSrVSwPUPQ5z1Bjb4U7v7txJ4O0G9B9g2w40bmNfotPBiPaj3T1cp6DPN0TuC7IfoYUr4Tohy4rGqEVTLh9pMUUeExc8lYSQMo/ss94kvCnAOIK+JUtTEz0A1jVeS4KsFNu5vIm+i91diq7G6MZLHxYG3bRdsxmTB8SR7W2qxLfnQlOYdE2/laZiBpzL81Bzma4KgpApGNru6/2GTer8NjdMRnVdhdMRzx51c8+MkKSeK5IShaCIZFPiLutslTvXcBVr8v4raTjUyTO/eYHf2vxb9c1lGI+/LHA5EVaJlMq6+NEumMlrHWeiWq/lVvRPuv7J8ZiEVwYFvutr+iXPOZ/davcF3w3QOsImatvh43abReLbxqWQsvURErzpryr/9utl+lGHTkzmpnsfQcUisTwTyiUF+V5148J42mqpBk6pTmtnEVsgyPTVyAdMkD/82X4IdBuBKfQEZRQeDRhn4sx4wnizWXQ83/WKRYw13HesULmFMFdpxCUS5RpcXmxnAXk2ks2zdlQZb3z3lE6VzWafUxdzFUwZfFiHcE9TgyE2v9Y6EmsyCuWsfdTywQW9hZJJ19ObjUumDyyjYDN0h6pwvtRxZXp1vWgV039K0htCMt6gQK8KeDLTM7hyqI3SeB 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: David Hildenbrand writes: > On 16.02.24 10:51, Kairui Song wrote: >> From: Kairui Song >> When skipping swapcache for SWP_SYNCHRONOUS_IO, if two or more >> threads >> swapin the same entry at the same time, they get different pages (A, B). >> Before one thread (T0) finishes the swapin and installs page (A) >> to the PTE, another thread (T1) could finish swapin of page (B), >> swap_free the entry, then swap out the possibly modified page >> reusing the same entry. It breaks the pte_same check in (T0) because >> PTE value is unchanged, causing ABA problem. Thread (T0) will >> install a stalled page (A) into the PTE and cause data corruption. >> One possible callstack is like this: >> CPU0 CPU1 >> ---- ---- >> do_swap_page() do_swap_page() with same entry >> >> >> swap_read_folio() <- read to page A swap_read_folio() <- read to page B >> >> ... set_pte_at() >> swap_free() <- entry is free >> >> >> pte_same() <- Check pass, PTE seems >> unchanged, but page A >> is stalled! >> swap_free() <- page B content lost! >> set_pte_at() <- staled page A installed! >> And besides, for ZRAM, swap_free() allows the swap device to discard >> the entry content, so even if page (B) is not modified, if >> swap_read_folio() on CPU0 happens later than swap_free() on CPU1, >> it may also cause data loss. >> To fix this, reuse swapcache_prepare which will pin the swap entry >> using >> the cache flag, and allow only one thread to pin it. Release the pin >> after PT unlocked. Racers will simply wait since it's a rare and very >> short event. A schedule() call is added to avoid wasting too much CPU >> or adding too much noise to perf statistics >> Other methods like increasing the swap count don't seem to be a good >> idea after some tests, that will cause racers to fall back to use the >> swap cache again. Parallel swapin using different methods leads to >> a much more complex scenario. >> Reproducer: >> This race issue can be triggered easily using a well constructed >> reproducer and patched brd (with a delay in read path) [1]: >> With latest 6.8 mainline, race caused data loss can be observed >> easily: >> $ gcc -g -lpthread test-thread-swap-race.c && ./a.out >> Polulating 32MB of memory region... >> Keep swapping out... >> Starting round 0... >> Spawning 65536 workers... >> 32746 workers spawned, wait for done... >> Round 0: Error on 0x5aa00, expected 32746, got 32743, 3 data loss! >> Round 0: Error on 0x395200, expected 32746, got 32743, 3 data loss! >> Round 0: Error on 0x3fd000, expected 32746, got 32737, 9 data loss! >> Round 0 Failed, 15 data loss! >> This reproducer spawns multiple threads sharing the same memory >> region >> using a small swap device. Every two threads updates mapped pages one by >> one in opposite direction trying to create a race, with one dedicated >> thread keep swapping out the data out using madvise. >> The reproducer created a reproduce rate of about once every 5 >> minutes, >> so the race should be totally possible in production. >> After this patch, I ran the reproducer for over a few hundred rounds >> and no data loss observed. >> Performance overhead is minimal, microbenchmark swapin 10G from 32G >> zram: >> Before: 10934698 us >> After: 11157121 us >> Non-direct: 13155355 us (Dropping SWP_SYNCHRONOUS_IO flag) >> Fixes: 0bcac06f27d7 ("mm, swap: skip swapcache for swapin of >> synchronous device") >> Link: https://github.com/ryncsn/emm-test-project/tree/master/swap-stress-race [1] >> Reported-by: "Huang, Ying" >> Closes: https://lore.kernel.org/lkml/87bk92gqpx.fsf_-_@yhuang6-desk2.ccr.corp.intel.com/ >> Signed-off-by: Kairui Song >> Cc: stable@vger.kernel.org >> --- >> Update from V2: >> - Add a schedule() if raced to prevent repeated page faults wasting CPU >> and add noise to perf statistics. >> - Use a bool to state the special case instead of reusing existing >> variables fixing error handling [Minchan Kim]. >> V2: >> https://lore.kernel.org/all/20240206182559.32264-1-ryncsn@gmail.com/ >> Update from V1: >> - Add some words on ZRAM case, it will discard swap content on swap_free so the race window is a bit different but cure is the same. [Barry Song] >> - Update comments make it cleaner [Huang, Ying] >> - Add a function place holder to fix CONFIG_SWAP=n built [SeongJae Park] >> - Update the commit message and summary, refer to SWP_SYNCHRONOUS_IO instead of "direct swapin path" [Yu Zhao] >> - Update commit message. >> - Collect Review and Acks. >> V1: >> https://lore.kernel.org/all/20240205110959.4021-1-ryncsn@gmail.com/ >> include/linux/swap.h | 5 +++++ >> mm/memory.c | 20 ++++++++++++++++++++ >> mm/swap.h | 5 +++++ >> mm/swapfile.c | 13 +++++++++++++ >> 4 files changed, 43 insertions(+) >> diff --git a/include/linux/swap.h b/include/linux/swap.h >> index 4db00ddad261..8d28f6091a32 100644 >> --- a/include/linux/swap.h >> +++ b/include/linux/swap.h >> @@ -549,6 +549,11 @@ static inline int swap_duplicate(swp_entry_t swp) >> return 0; >> } >> +static inline int swapcache_prepare(swp_entry_t swp) >> +{ >> + return 0; >> +} >> + >> static inline void swap_free(swp_entry_t swp) >> { >> } >> diff --git a/mm/memory.c b/mm/memory.c >> index 7e1f4849463a..7059230d0a54 100644 >> --- a/mm/memory.c >> +++ b/mm/memory.c >> @@ -3799,6 +3799,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) >> struct page *page; >> struct swap_info_struct *si = NULL; >> rmap_t rmap_flags = RMAP_NONE; >> + bool need_clear_cache = false; >> bool exclusive = false; >> swp_entry_t entry; >> pte_t pte; >> @@ -3867,6 +3868,20 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) >> if (!folio) { >> if (data_race(si->flags & SWP_SYNCHRONOUS_IO) && >> __swap_count(entry) == 1) { >> + /* >> + * Prevent parallel swapin from proceeding with >> + * the cache flag. Otherwise, another thread may >> + * finish swapin first, free the entry, and swapout >> + * reusing the same entry. It's undetectable as >> + * pte_same() returns true due to entry reuse. >> + */ >> + if (swapcache_prepare(entry)) { >> + /* Relax a bit to prevent rapid repeated page faults */ >> + schedule(); >> + goto out; >> + } >> + need_clear_cache = true; >> + > > I took a closer look at __read_swap_cache_async() and it essentially > does something similar. > > Instead of returning, it keeps retrying until it finds that > swapcache_prepare() fails for another reason than -EEXISTS (e.g., > freed concurrently) or it finds the entry in the swapcache. > > So if you would succeed here on a freed+reused swap entry, > __read_swap_cache_async() would simply retry. > > It spells that out: > > /* > * We might race against __delete_from_swap_cache(), and > * stumble across a swap_map entry whose SWAP_HAS_CACHE > * has not yet been cleared. Or race against another > * __read_swap_cache_async(), which has set SWAP_HAS_CACHE > * in swap_map, but not yet added its folio to swap cache. > */ > > Whereby we could not race against this code here as well where we > speculatively set SWAP_HAS_CACHE and might never add something to the swap > cache. > > > I'd probably avoid the wrong returns and do something even closer to > __read_swap_cache_async(). > > while (true) { > /* > * Fake that we are trying to insert a page into the swapcache, to > * serialize against concurrent threads wanting to do the same. > * [more from your description] > */ > ret = swapcache_prepare(entry); > if (likely(!ret) > /* > * Move forward with swapin, we'll recheck if the PTE hasn't > * changed later. > */ > break; > else if (ret != -EEXIST) > goto out; The swap entry may be kept in swap cache for long time. For example, it may be read into swap cache via MADV_WILLNEED. -- Best Regards, Huang, Ying > > /* > * See __read_swap_cache_async(). We might either have raced against > * another thread, or the entry could have been freed and reused in the > * meantime. Make sure that the PTE did not change, to detect freeing. > */ > vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, > vmf->address, &vmf->ptl); > if (!vmf->pte || !pte_same(ptep_get(vmf->pte), vmf->orig_pte)) > goto unlock; > > > schedule(); > } > > > > I was skeptical about the schedule(), but __read_swap_cache_async() does it > already because there is no better way to wait for the event to happen. > > With something like above you would no longer depend on the speed of schedule() to > determine how often you would retry the fault, which would likely make sense. > > I do wonder about the schedule() vs. schedule_timeout_uninterruptible(), though. > No expert on that area, do you have any idea?