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 1BAB3C4828D for ; Tue, 6 Feb 2024 03:22:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 970746B007B; Mon, 5 Feb 2024 22:22:13 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 920836B007D; Mon, 5 Feb 2024 22:22:13 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 79AD56B007E; Mon, 5 Feb 2024 22:22: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 69D9B6B007B for ; Mon, 5 Feb 2024 22:22:13 -0500 (EST) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 33B2BC0312 for ; Tue, 6 Feb 2024 03:22:13 +0000 (UTC) X-FDA: 81759930546.11.FE6D1A9 Received: from mail-lf1-f43.google.com (mail-lf1-f43.google.com [209.85.167.43]) by imf17.hostedemail.com (Postfix) with ESMTP id 4CDCD40002 for ; Tue, 6 Feb 2024 03:22:10 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=anDzVYgo; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf17.hostedemail.com: domain of ryncsn@gmail.com designates 209.85.167.43 as permitted sender) smtp.mailfrom=ryncsn@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707189731; 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:dkim-signature; bh=nrdIhk8ATB609TluZLy9x7HB4Q7Va08Ieexvs7G06Tc=; b=rb4+GHO8ozYiBNzla7MpNRUtstw8t0c9H9s1ExvA6GJbMLOJg5L3tXp1kkR6X8cxc3wtQp 95a0DEtmeOuUxlLc2ClF+GBY+FFFRGm4+iVz7tq0gl3KdupQsrY5DGBbO8GlpGLusoGyM/ zHFJspD0+UB7CxzlVAOTzmkghjeaUCE= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=anDzVYgo; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf17.hostedemail.com: domain of ryncsn@gmail.com designates 209.85.167.43 as permitted sender) smtp.mailfrom=ryncsn@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707189731; a=rsa-sha256; cv=none; b=Y8LqCnIjK8HuYkLhoKTVUjFbDrD95rihi0FUOjXsNKHGqf1dk/nAQzpaM9zfVTLs5tR14/ sgSYbhYZectbAAr8ydYSctdfhospNAC2Fb4glJ/AXvzxFe2RXwV2BTAC/3Q8Q9taAD/XyD ME/JkabKM8ayzht7QGNJWE053BntIQE= Received: by mail-lf1-f43.google.com with SMTP id 2adb3069b0e04-51117bfd452so8628082e87.3 for ; Mon, 05 Feb 2024 19:22:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1707189729; x=1707794529; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=nrdIhk8ATB609TluZLy9x7HB4Q7Va08Ieexvs7G06Tc=; b=anDzVYgo/ykFOyDHBzIlnSuc5PElVi0WOUGoBvTDZwFDwIvQFpf8xOvHOJBEjyBlJP KL/VlOSe7hzsLiNj9+ibdIW0EmMmwI25Q2We+K4vAPF9ea88bTl+uUWsxHDUvkqe+coX /MZ5EQfBfAuhvYHIIVPWl7/wR7hQEQzxh/Xyr8a1N3Q+5iTi5ATQmPhGTpE3aMmyGLem IFLgddVaJwdEm3ztNJyJ41jxFcmI9A4StWPVizlhwnPZ+L5K4O3ua0/wFRmOGecyz3Ao A7rBIOQHy5eZkU4DUyLYpnerpdpX3VE6LCbpTvwv1Wl+EeXqL0QLV54EXzlU6e1xa9MN 4SCQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707189729; x=1707794529; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=nrdIhk8ATB609TluZLy9x7HB4Q7Va08Ieexvs7G06Tc=; b=dPpxk3lqn2BE7HI5j5D+UpZmJdAVFW3cAi+qbHUueuLC/BLDJTRt4rxgL1RMNswgYE 4KYHcsT/+mZTJBDq5p3hTBrP9tSemQrlmkMdxuYfj+iroUCe8RrXQFgf8MBO292PG1NR TbPMMp4RcFv/SO8SRpFmx0zEPsRsx+VTep1G2sivfoH+jYH4PFKEe2CUGxCh8RxalV2n ozaBv38tnBwZjsfNuLK41ou8soTQjxeowYlV0Ik+9f64YLIjjBU1qrVedIB543djfYoZ vpTbVJrLmzs4BDx2hK3dVopuTEUXMvFeL15c/CHz17U41bC/5syvfZ/Dh7RmuTxDC+Tx wXuw== X-Gm-Message-State: AOJu0YwAW9Y3ZfRLUDVLYv0V1modiaoN5z/TvH04wlEXfnKsn12dSABa GuinYC/RCSK6Hz/RWdvPq/AcPM3CgqRD8eIxsI7kWBI1sVkzQdNRfj+bT0XnjKJ5u8+ZXFcZHGu +99Y9KsxSv8t7F6UrhbMW8S3mN7I= X-Google-Smtp-Source: AGHT+IEXXDk2KhXkTGg/n364JSzsq7BErLLprh4BWymFF7C72GAlInbn9sKI5+ewIssHRoUNFa1csYhmoWut4gsXMSs= X-Received: by 2002:a2e:b54a:0:b0:2d0:9cc6:96fb with SMTP id a10-20020a2eb54a000000b002d09cc696fbmr583799ljn.44.1707189729009; Mon, 05 Feb 2024 19:22:09 -0800 (PST) MIME-Version: 1.0 References: <20240205110959.4021-1-ryncsn@gmail.com> <871q9q1imi.fsf@yhuang6-desk2.ccr.corp.intel.com> In-Reply-To: <871q9q1imi.fsf@yhuang6-desk2.ccr.corp.intel.com> From: Kairui Song Date: Tue, 6 Feb 2024 11:21:51 +0800 Message-ID: Subject: Re: [PATCH] mm/swap: fix race condition in direct swapin path To: "Huang, Ying" Cc: linux-mm@kvack.org, Andrew Morton , Chris Li , Minchan Kim , Hugh Dickins , Johannes Weiner , Matthew Wilcox , Michal Hocko , Yosry Ahmed , David Hildenbrand , Yu Zhao , linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4CDCD40002 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: a6kxwnti9kiydaqqoemwxrs8cxw57hqr X-HE-Tag: 1707189730-157870 X-HE-Meta: U2FsdGVkX1+OZAVswilJCOJjQuvP5SgZeKUf42IW7kY+vmOsPCYIn4pbzjn6kfVS/ezdNg5Hffn6CzZy2nzPB2OU1cJ0LDnF/+d6q8nfclNmURJGcreDLlG81pp76SGHySX+Ow9G7CAENWTqDkjIcVsqhWtsKr5fI6T4UDaNrdRpNejpGE30kV0YBQjJLsC0VFZvHtfBjgtQnH4fA1YpjHwBrLUeuxrQJ+ZGopJXQ+jtvmUmp1a7Tv67GDYtgQqAx2ebB8awHDFXPtKcAOjsO7qOrBUt9Dx6MI6RJnakCwP9HnscBIbKML/h4LpmUdEY0NYgFpsz39iqUdG4WjtNcjdfraIhsvXxXUy1/ZFJoIeUPFKgOxkuRJvfbXKaq9lNpLI1Kh1ZC2KKeGoY9Md2S+bDd9XC5aZl+5vt0OgvZ5eRz2Upv9tILn0ODifcpYg2ejK82ubdtD4ouJFIbjMQWJjrKifW/FOEiJA7BKZTB4JRJGVa8GvCTRhC5/Oidp93LP+iw93Jd1YWukwEkA4DHeNlUcobor9exAz7FCbH68l2CqEngaUoz/w1CY4y/cXvVH92SHXR1g2BqUA2iE0janSSf04y2apwibwhnrBuSW0Xjvx6xuDCl/BhRq9TEmp5ju/yxzZqmjyJMf/NNAZx1Ka2Nx0bFHj06KDNHvu7uvAa2vC1uZvkI/1y+jLipRmLIh3QNlH8/8G9uoI01ac/nAdjhgYFWXF5Af0+2cKEtECk/IvvUHRRXGAFmFTAcat+6dsCKP2HFG12KGfbSKiGzKpeGHRKw/bg+n5vnP7tx7912ZOlRj45fGCqGwHE+APs6SYLsd1Ue49n2vya5lHzp2qjxAj4DcKb3bBLLxUMtZYZ3vDGdN7BgmaLpKGfJCLhV5m6EpKShK7PIvggtMjK2zBjN3lKfwCNeViTWkAsd7V6wOH2ewd+c7TXOdX+vajhSpO4Utg+V929KC+QYda pXi3/TR3 1eO+kXmpDdavZYMCX70ZXXS/XJtpJehJbjBRpu0i7YbIpTDYpiLGFZqoXkLVLpr64CVoomugymSrVhJ9D14PwyVddAkvqlWOgmevD83G2WQ6MF27QzNPWwHjgQtHIDNQMoR+KUEbnChnPOJCbEzBxbtGV30AXtV5Z1LrSTRUWVfFKA2OHBMAQYUAXvkIuL5gYY2dAHwfXXbyP2wpfEVg4PSrCqQJhAvRBZWO7mUnB4oIRYgljImrAisY9fpg2oM8nO0dn/qaA3qpPXOU4gNUDsz20xkq7BQL8s17XqsBAGer5hucXvTFeexMVr4MYCSaKgfL6Ik1yi9sQbzrdE4h2SnWE9K9bLQdyLZvS3xJG1ocmmc+GhFAcuwmcoNPfVy9OHrz1IoaFQ3yXk2sWe+tm/jC8OFci8Uc5YyQOxnRXVCn7kUhUgsNA1FUc+dmfMCsMYzpt9Tmr/3HktgaL/jFytvkhp6/UoElQvwW5qsrYbJsL1TtozIj01RT2+ayXBRUWJa7m 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 Tue, Feb 6, 2024 at 9:35=E2=80=AFAM Huang, Ying w= rote: > > Kairui Song writes: > > > From: Kairui Song > > > > In the direct swapin path, when two or more threads swapin the same ent= ry > > at the same time, they get different pages (A, B) because swap cache is > > skipped. 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 modify and swap-out the page again, using the > > same entry. It break the pte_same check because PTE value is unchanged, > > causing ABA problem. Then thread (T0) will then install the stalled pag= e > > (A) into the PTE so new data in page (B) is lost, one possible callstac= k > > is like this: > > > > CPU0 CPU1 > > ---- ---- > > do_swap_page() do_swap_page() with same entry > > > > > > swap_readpage() <- read to page A swap_readpage() <- read to page B > > > > ... set_pte_at() > > swap_free() <- Now the entry is fre= ed. > > > > > > 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! > > > > To fix this, reuse swapcache_prepare which will pin the swap entry usin= g > > the cache flag, and allow only one thread to pin it. Release the pin > > after PT unlocked. Racers will simply busy wait since it's a rare > > and very short event. > > > > 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 the > > cached swapin path, two swapin path being used at the same time > > 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 b= y > > 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 synchronou= s device") > > Link: https://github.com/ryncsn/emm-test-project/tree/master/swap-stres= s-race [1] > > Signed-off-by: Kairui Song > > Reported-by: "Huang, Ying" Of course :), wasn't sure about how to add your credit, will add this to V2= . > > --- > > Huge thanks to Huang Ying and Chris Li for help finding this issue! > > > > mm/memory.c | 19 +++++++++++++++++++ > > mm/swap.h | 5 +++++ > > mm/swapfile.c | 16 ++++++++++++++++ > > 3 files changed, 40 insertions(+) > > > > diff --git a/mm/memory.c b/mm/memory.c > > index 7e1f4849463a..fd7c55a292f1 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -3867,6 +3867,20 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > if (!folio) { > > if (data_race(si->flags & SWP_SYNCHRONOUS_IO) && > > __swap_count(entry) =3D=3D 1) { > > + /* > > + * With swap count =3D=3D 1, after we read the en= try, > > + * other threads could finish swapin first, free > > + * the entry, then swapout the modified page usin= g > > + * the same entry. Now the content we just read i= s > > + * stalled, and it's undetectable as pte_same() > > + * returns true due to entry reuse. > > + * > > + * So pin the swap entry using the cache flag eve= n > > "pin" doesn't sound intuitive here. I know that the swap entry will not > be freed with this. But now, the parallel swap in will busy waiting. > So, I suggest to say, > > Prevent parallel swapin from proceeding with the cache flag. Otherwise, > it may swapin first, free the entry, then swapout the modified page > using the same entry ... Good suggestion. > > > + * cache is not used. > > + */ > > + if (swapcache_prepare(entry)) > > + goto out; > > + > > /* skip swapcache */ > > folio =3D vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0= , > > vma, vmf->address, false)= ; > > @@ -4116,6 +4130,9 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > unlock: > > if (vmf->pte) > > pte_unmap_unlock(vmf->pte, vmf->ptl); > > + /* Clear the swap cache pin for direct swapin after PTL unlock */ > > + if (folio && !swapcache) > > + swapcache_clear(si, entry); > > out: > > if (si) > > put_swap_device(si); > > @@ -4124,6 +4141,8 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > if (vmf->pte) > > pte_unmap_unlock(vmf->pte, vmf->ptl); > > out_page: > > + if (!swapcache) > > + swapcache_clear(si, entry); > > folio_unlock(folio); > > out_release: > > folio_put(folio); > > diff --git a/mm/swap.h b/mm/swap.h > > index 758c46ca671e..fc2f6ade7f80 100644 > > --- a/mm/swap.h > > +++ b/mm/swap.h > > @@ -41,6 +41,7 @@ void __delete_from_swap_cache(struct folio *folio, > > void delete_from_swap_cache(struct folio *folio); > > void clear_shadow_from_swap_cache(int type, unsigned long begin, > > unsigned long end); > > +void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry); > > struct folio *swap_cache_get_folio(swp_entry_t entry, > > struct vm_area_struct *vma, unsigned long addr); > > struct folio *filemap_get_incore_folio(struct address_space *mapping, > > @@ -97,6 +98,10 @@ static inline int swap_writepage(struct page *p, str= uct writeback_control *wbc) > > return 0; > > } > > > > +static inline void swapcache_clear(struct swap_info_struct *si, swp_en= try_t entry) > > +{ > > +} > > + > > static inline struct folio *swap_cache_get_folio(swp_entry_t entry, > > struct vm_area_struct *vma, unsigned long addr) > > { > > diff --git a/mm/swapfile.c b/mm/swapfile.c > > index 556ff7347d5f..f7d4ad152a7f 100644 > > --- a/mm/swapfile.c > > +++ b/mm/swapfile.c > > @@ -3365,6 +3365,22 @@ int swapcache_prepare(swp_entry_t entry) > > return __swap_duplicate(entry, SWAP_HAS_CACHE); > > } > > > > +/* > > + * Clear the cache flag and release pinned entry. > > Even if we will keep "pin" in above comments, this is hard to be > understood for reader. Need a little more explanation like "release > pinned entry for device with SWP_SYNCHRONOUS_IO. > > Or, just remove the comments. We have comments in calling site already. Then I prefer to remove this, there is only one caller, it should be easy to understand. > > + */ > > +void swapcache_clear(struct swap_info_struct *si, swp_entry_t entry) > > +{ > > + struct swap_cluster_info *ci; > > + unsigned long offset =3D swp_offset(entry); > > + unsigned char usage; > > + > > + ci =3D lock_cluster_or_swap_info(si, offset); > > + usage =3D __swap_entry_free_locked(si, offset, SWAP_HAS_CACHE); > > + unlock_cluster_or_swap_info(si, ci); > > + if (!usage) > > + free_swap_slot(entry); > > +} > > + > > struct swap_info_struct *swp_swap_info(swp_entry_t entry) > > { > > return swap_type_to_swap_info(swp_type(entry)); > > Otherwise it looks good for me, Thanks! > > Reviewed-by: "Huang, Ying" Thanks for the review.