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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 377FAC7EE23 for ; Tue, 16 May 2023 02:35:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229706AbjEPCfg (ORCPT ); Mon, 15 May 2023 22:35:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50030 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229867AbjEPCfd (ORCPT ); Mon, 15 May 2023 22:35:33 -0400 Received: from mail-pl1-x62b.google.com (mail-pl1-x62b.google.com [IPv6:2607:f8b0:4864:20::62b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 598837298 for ; Mon, 15 May 2023 19:35:31 -0700 (PDT) Received: by mail-pl1-x62b.google.com with SMTP id d9443c01a7336-1aae90f5ebcso772885ad.1 for ; Mon, 15 May 2023 19:35:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1684204531; x=1686796531; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=LMBjYQfNrE6XjpDNQgsEFZW5LIEYARmq7ri2dsjhSck=; b=Y9M5TADWZmKtWP/pjJc/AL6uypfuLC3F175I1lHhKkjmCaaG0uqoZBHO8dNAmzCurF mSuWZAiDqzx99dwMuAC3x1Sm3in8ctWriOZNyqdyM7/l2GYAbTStuAUCVC/TGugUIbXB VIyiv88/gu3mEghvmAZsxlsqlEgqy5tEtiv5+5GzubDW94Sn9J2GxUJ22Olh+HZPtgGG O9YC/yXBcZOdAxFeBxjmQInlps6eDlKo0aZymWEcgDqLH/dXkhtGv5b5bRv4BUdiPtI3 ZrS7uKquQVk1kRRSUApyhR44WIY4nh6IJeRqRjIPyPBkGp/x0+5jbKTcgKFcbPEEePwf SGxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684204531; x=1686796531; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=LMBjYQfNrE6XjpDNQgsEFZW5LIEYARmq7ri2dsjhSck=; b=aVCPweKW6d9cret5nKgkbVV54dUHkF+XCHKcSqppWAfxChA1NPT5wZEnifp161FUIb L4oQtUMwo8syvDHPWbp+bk/iNzSAXndsYfg788f9UgpQYnQoMlXiXt+MG7Ys3N6D21Ne CwHy/m5t5yCqKBR3SijLIguTO0UiWtoZXJejtolhVbv/GMp+cJYuZtnOPQF+kfzKvUJV Ae2954c5TbTH+0qUm5gtTsrIGjvB4cNUUi25EGcFF/JocCpr2LUp/dTBlFr71pEcmwBi 0cT0y8vXxOGyEBKE0KTSKdI9IAtXHivDGZFPnI9eew0pnWiEr4D5d6KfDhooPf9JBKFj yOEA== X-Gm-Message-State: AC+VfDwBclRp/xXh8cP9R4VV0rppU/IjGQhBNWJFJT5J7wMCghE4Cy1+ eA3RolI94S7ir1KzD3gBm8Ydww== X-Google-Smtp-Source: ACHHUZ5JXRMgy2Sy3iucvOjzvE5J55mKVFuuRALHbThX2oA4vZEbQg9phtDXVle1ISQ5G6F2JuLD2g== X-Received: by 2002:a17:902:c1d3:b0:19c:c5d4:afd2 with SMTP id c19-20020a170902c1d300b0019cc5d4afd2mr9124plc.11.1684204530548; Mon, 15 May 2023 19:35:30 -0700 (PDT) Received: from google.com ([2620:15c:2d3:205:c825:9c0b:b4be:8ee4]) by smtp.gmail.com with ESMTPSA id t23-20020a634457000000b0051afa49e07asm12283006pgk.50.2023.05.15.19.35.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 15 May 2023 19:35:30 -0700 (PDT) Date: Mon, 15 May 2023 19:35:24 -0700 From: Peter Collingbourne To: David Hildenbrand Cc: Catalin Marinas , Qun-wei Lin =?utf-8?B?KOael+e+pOW0tCk=?= , linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, "surenb@google.com" , Chinwen Chang =?utf-8?B?KOW8temMpuaWhyk=?= , "kasan-dev@googlegroups.com" , Kuan-Ying Lee =?utf-8?B?KOadjuWGoOepjik=?= , Casper Li =?utf-8?B?KOadjuS4reamrik=?= , "gregkh@linuxfoundation.org" , vincenzo.frascino@arm.com, Alexandru Elisei , will@kernel.org, eugenis@google.com, Steven Price , stable@vger.kernel.org Subject: Re: [PATCH 1/3] mm: Move arch_do_swap_page() call to before swap_free() Message-ID: References: <20230512235755.1589034-1-pcc@google.com> <20230512235755.1589034-2-pcc@google.com> <7471013e-4afb-e445-5985-2441155fc82c@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org On Mon, May 15, 2023 at 05:16:09PM -0700, Peter Collingbourne wrote: > On Sat, May 13, 2023 at 05:29:53AM +0200, David Hildenbrand wrote: > > On 13.05.23 01:57, Peter Collingbourne wrote: > > > Commit c145e0b47c77 ("mm: streamline COW logic in do_swap_page()") moved > > > the call to swap_free() before the call to set_pte_at(), which meant that > > > the MTE tags could end up being freed before set_pte_at() had a chance > > > to restore them. One other possibility was to hook arch_do_swap_page(), > > > but this had a number of problems: > > > > > > - The call to the hook was also after swap_free(). > > > > > > - The call to the hook was after the call to set_pte_at(), so there was a > > > racy window where uninitialized metadata may be exposed to userspace. > > > This likely also affects SPARC ADI, which implements this hook to > > > restore tags. > > > > > > - As a result of commit 1eba86c096e3 ("mm: change page type prior to > > > adding page table entry"), we were also passing the new PTE as the > > > oldpte argument, preventing the hook from knowing the swap index. > > > > > > Fix all of these problems by moving the arch_do_swap_page() call before > > > the call to free_page(), and ensuring that we do not set orig_pte until > > > after the call. > > > > > > Signed-off-by: Peter Collingbourne > > > Suggested-by: Catalin Marinas > > > Link: https://linux-review.googlesource.com/id/I6470efa669e8bd2f841049b8c61020c510678965 > > > Cc: # 6.1 > > > Fixes: ca827d55ebaa ("mm, swap: Add infrastructure for saving page metadata on swap") > > > Fixes: 1eba86c096e3 ("mm: change page type prior to adding page table entry") > > > > I'm confused. You say c145e0b47c77 changed something (which was after above > > commits), indicate that it fixes two other commits, and indicate "6.1" as > > stable which does not apply to any of these commits. > > Sorry, the situation is indeed a bit confusing. > > - In order to make the arch_do_swap_page() hook suitable for fixing the > bug introduced by c145e0b47c77, patch 1 addresses a number of issues, > including fixing bugs introduced by ca827d55ebaa and 1eba86c096e3, > but we haven't fixed the c145e0b47c77 bug yet, so there's no Fixes: > tag for it yet. > > - Patch 2, relying on the fixes in patch 1, makes MTE install an > arch_do_swap_page() hook (indirectly, by making arch_swap_restore() > also hook arch_do_swap_page()), thereby fixing the c145e0b47c77 bug. > > - 6.1 is the first stable version in which all 3 commits in my Fixes: tags > are present, so that is the version that I've indicated in my stable > tag for this series. In theory patch 1 could be applied to older kernel > versions, but it wouldn't fix any problems that we are facing with MTE > (because it only fixes problems relating to the arch_do_swap_page() > hook, which older kernel versions don't hook with MTE), and there are > some merge conflicts if we go back further anyway. If the SPARC folks > (the previous only user of this hook) want to fix these issues with ADI, > they can propose their own backport. > > > > @@ -3959,7 +3960,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > > VM_BUG_ON(!folio_test_anon(folio) || > > > (pte_write(pte) && !PageAnonExclusive(page))); > > > set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte); > > > - arch_do_swap_page(vma->vm_mm, vma, vmf->address, pte, vmf->orig_pte); > > > folio_unlock(folio); > > > if (folio != swapcache && swapcache) { > > > > > > You are moving the folio_free_swap() call after the folio_ref_count(folio) > > == 1 check, which means that such (previously) swapped pages that are > > exclusive cannot be detected as exclusive. > > Ack. I will fix this in v2. I gave this some thought and concluded that the added complexity needed to make this hook suitable for arm64 without breaking sparc probably isn't worth it in the end, and as I explained in patch 2, sparc ought to be moving away from this hook anyway. So in v2 I replaced patches 1 and 2 with a patch that adds a direct call to the arch_swap_restore() hook before the call to swap_free(). Peter 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4189EC77B7D for ; Tue, 16 May 2023 02:36:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=DOrJ8cj30h1AWVIk4Vqr9VdIKQTsWToptm6clcBwLl8=; b=3U5WiobtKQjvHe IgTqM0q4H2bkmgpNKPF4/SaWtntuTRNSmo267wyF7hL54rKSP0gz9dgviELoY4IEnWSo6RW2Bs0Xs GvMdAyTTh3ZjuE7necyRkAce5bIXCabMR7nOw8+b0siYplubZmjhZunG6zStQUnHemsyX7p5hS1YA 9xqWGMxGIvb7gwv0imf+II9yfRu0zS7A7dy6d+5O0mAGCeyVshCrtQ/5dK/UuwFXMYSPs4hfxFaOh 8009ScwFwtAZJSgaNT5aBZZ2oIN4rxkHcW6OjMov2nwmJrzoqmgH9OUPA1N2FpeXW6mh0WWRIr6QF /uyGxVkDlhqYv7a+JzAQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pykXK-0049QS-28; Tue, 16 May 2023 02:35:38 +0000 Received: from mail-pl1-x636.google.com ([2607:f8b0:4864:20::636]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pykXG-0049Nr-1c for linux-arm-kernel@lists.infradead.org; Tue, 16 May 2023 02:35:36 +0000 Received: by mail-pl1-x636.google.com with SMTP id d9443c01a7336-1aae90f5ebcso772835ad.1 for ; Mon, 15 May 2023 19:35:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1684204531; x=1686796531; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=LMBjYQfNrE6XjpDNQgsEFZW5LIEYARmq7ri2dsjhSck=; b=Y9M5TADWZmKtWP/pjJc/AL6uypfuLC3F175I1lHhKkjmCaaG0uqoZBHO8dNAmzCurF mSuWZAiDqzx99dwMuAC3x1Sm3in8ctWriOZNyqdyM7/l2GYAbTStuAUCVC/TGugUIbXB VIyiv88/gu3mEghvmAZsxlsqlEgqy5tEtiv5+5GzubDW94Sn9J2GxUJ22Olh+HZPtgGG O9YC/yXBcZOdAxFeBxjmQInlps6eDlKo0aZymWEcgDqLH/dXkhtGv5b5bRv4BUdiPtI3 ZrS7uKquQVk1kRRSUApyhR44WIY4nh6IJeRqRjIPyPBkGp/x0+5jbKTcgKFcbPEEePwf SGxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684204531; x=1686796531; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=LMBjYQfNrE6XjpDNQgsEFZW5LIEYARmq7ri2dsjhSck=; b=TojPHTE2HGddJikHXznstZY1+USWediPb6jVjaF5X21yy+ChTparWAVWLIMSJrayrJ YcA3PzAkER+IAwnsd2c5yM8abZjYpkw1DSfMeVZ8hIl3egcBezI4HbXCeej+HljGkl/b 5l1l9AndI6KiBe06A4VNGCCrm5xwfLH1sdR5nGTFPcnLO65LpMqnC7z5C/Bhcqhdpsh5 8KNDve1T1UAxLO3lCuoTqbplGAiTZnPgAdW5tgo5FVP04eXtqftJt04KrTOwuqSWg9Tf FQIkC9Dn4PcXj/kzWyt2Iz9J2yf4slKlQpjhxRSpBoa/XX3s7uRs/8sFURbdkEHwvtLn qx8w== X-Gm-Message-State: AC+VfDwNh/NNb7FAKmfdmPkv3PsoOgQd5iPgVGnNih+YDEoLR7HXwi+L Drt3E/5x34PM7ToVrITChcU12w== X-Google-Smtp-Source: ACHHUZ5JXRMgy2Sy3iucvOjzvE5J55mKVFuuRALHbThX2oA4vZEbQg9phtDXVle1ISQ5G6F2JuLD2g== X-Received: by 2002:a17:902:c1d3:b0:19c:c5d4:afd2 with SMTP id c19-20020a170902c1d300b0019cc5d4afd2mr9124plc.11.1684204530548; Mon, 15 May 2023 19:35:30 -0700 (PDT) Received: from google.com ([2620:15c:2d3:205:c825:9c0b:b4be:8ee4]) by smtp.gmail.com with ESMTPSA id t23-20020a634457000000b0051afa49e07asm12283006pgk.50.2023.05.15.19.35.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 15 May 2023 19:35:30 -0700 (PDT) Date: Mon, 15 May 2023 19:35:24 -0700 From: Peter Collingbourne To: David Hildenbrand Cc: Catalin Marinas , Qun-wei Lin =?utf-8?B?KOael+e+pOW0tCk=?= , linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, "surenb@google.com" , Chinwen Chang =?utf-8?B?KOW8temMpuaWhyk=?= , "kasan-dev@googlegroups.com" , Kuan-Ying Lee =?utf-8?B?KOadjuWGoOepjik=?= , Casper Li =?utf-8?B?KOadjuS4reamrik=?= , "gregkh@linuxfoundation.org" , vincenzo.frascino@arm.com, Alexandru Elisei , will@kernel.org, eugenis@google.com, Steven Price , stable@vger.kernel.org Subject: Re: [PATCH 1/3] mm: Move arch_do_swap_page() call to before swap_free() Message-ID: References: <20230512235755.1589034-1-pcc@google.com> <20230512235755.1589034-2-pcc@google.com> <7471013e-4afb-e445-5985-2441155fc82c@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230515_193534_540096_EF5AF899 X-CRM114-Status: GOOD ( 38.68 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, May 15, 2023 at 05:16:09PM -0700, Peter Collingbourne wrote: > On Sat, May 13, 2023 at 05:29:53AM +0200, David Hildenbrand wrote: > > On 13.05.23 01:57, Peter Collingbourne wrote: > > > Commit c145e0b47c77 ("mm: streamline COW logic in do_swap_page()") moved > > > the call to swap_free() before the call to set_pte_at(), which meant that > > > the MTE tags could end up being freed before set_pte_at() had a chance > > > to restore them. One other possibility was to hook arch_do_swap_page(), > > > but this had a number of problems: > > > > > > - The call to the hook was also after swap_free(). > > > > > > - The call to the hook was after the call to set_pte_at(), so there was a > > > racy window where uninitialized metadata may be exposed to userspace. > > > This likely also affects SPARC ADI, which implements this hook to > > > restore tags. > > > > > > - As a result of commit 1eba86c096e3 ("mm: change page type prior to > > > adding page table entry"), we were also passing the new PTE as the > > > oldpte argument, preventing the hook from knowing the swap index. > > > > > > Fix all of these problems by moving the arch_do_swap_page() call before > > > the call to free_page(), and ensuring that we do not set orig_pte until > > > after the call. > > > > > > Signed-off-by: Peter Collingbourne > > > Suggested-by: Catalin Marinas > > > Link: https://linux-review.googlesource.com/id/I6470efa669e8bd2f841049b8c61020c510678965 > > > Cc: # 6.1 > > > Fixes: ca827d55ebaa ("mm, swap: Add infrastructure for saving page metadata on swap") > > > Fixes: 1eba86c096e3 ("mm: change page type prior to adding page table entry") > > > > I'm confused. You say c145e0b47c77 changed something (which was after above > > commits), indicate that it fixes two other commits, and indicate "6.1" as > > stable which does not apply to any of these commits. > > Sorry, the situation is indeed a bit confusing. > > - In order to make the arch_do_swap_page() hook suitable for fixing the > bug introduced by c145e0b47c77, patch 1 addresses a number of issues, > including fixing bugs introduced by ca827d55ebaa and 1eba86c096e3, > but we haven't fixed the c145e0b47c77 bug yet, so there's no Fixes: > tag for it yet. > > - Patch 2, relying on the fixes in patch 1, makes MTE install an > arch_do_swap_page() hook (indirectly, by making arch_swap_restore() > also hook arch_do_swap_page()), thereby fixing the c145e0b47c77 bug. > > - 6.1 is the first stable version in which all 3 commits in my Fixes: tags > are present, so that is the version that I've indicated in my stable > tag for this series. In theory patch 1 could be applied to older kernel > versions, but it wouldn't fix any problems that we are facing with MTE > (because it only fixes problems relating to the arch_do_swap_page() > hook, which older kernel versions don't hook with MTE), and there are > some merge conflicts if we go back further anyway. If the SPARC folks > (the previous only user of this hook) want to fix these issues with ADI, > they can propose their own backport. > > > > @@ -3959,7 +3960,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > > VM_BUG_ON(!folio_test_anon(folio) || > > > (pte_write(pte) && !PageAnonExclusive(page))); > > > set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte); > > > - arch_do_swap_page(vma->vm_mm, vma, vmf->address, pte, vmf->orig_pte); > > > folio_unlock(folio); > > > if (folio != swapcache && swapcache) { > > > > > > You are moving the folio_free_swap() call after the folio_ref_count(folio) > > == 1 check, which means that such (previously) swapped pages that are > > exclusive cannot be detected as exclusive. > > Ack. I will fix this in v2. I gave this some thought and concluded that the added complexity needed to make this hook suitable for arm64 without breaking sparc probably isn't worth it in the end, and as I explained in patch 2, sparc ought to be moving away from this hook anyway. So in v2 I replaced patches 1 and 2 with a patch that adds a direct call to the arch_swap_restore() hook before the call to swap_free(). Peter _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel