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 X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D62B6C64E7C for ; Wed, 2 Dec 2020 12:04:58 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 3DA06221FE for ; Wed, 2 Dec 2020 12:04:57 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3DA06221FE Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 5E6C46B0068; Wed, 2 Dec 2020 07:04:57 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 596F76B006C; Wed, 2 Dec 2020 07:04:57 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 45E816B006E; Wed, 2 Dec 2020 07:04:57 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0140.hostedemail.com [216.40.44.140]) by kanga.kvack.org (Postfix) with ESMTP id 2E9C86B0068 for ; Wed, 2 Dec 2020 07:04:57 -0500 (EST) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id DEC72824999B for ; Wed, 2 Dec 2020 12:04:56 +0000 (UTC) X-FDA: 77548210992.04.goat75_620aa7b273b3 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin04.hostedemail.com (Postfix) with ESMTP id C2D78800B667 for ; Wed, 2 Dec 2020 12:04:56 +0000 (UTC) X-HE-Tag: goat75_620aa7b273b3 X-Filterd-Recvd-Size: 8481 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf49.hostedemail.com (Postfix) with ESMTP for ; Wed, 2 Dec 2020 12:04:56 +0000 (UTC) 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 7B039101E; Wed, 2 Dec 2020 04:04:55 -0800 (PST) Received: from [192.168.0.130] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 086703F718; Wed, 2 Dec 2020 04:04:52 -0800 (PST) Subject: Re: [PATCH V2 1/2] mm/debug_vm_pgtable/basic: Add validation for dirtiness after write protect To: Steven Price , linux-mm@kvack.org, akpm@linux-foundation.org Cc: linux-kernel@vger.kernel.org, catalin.marinas@arm.com, christophe.leroy@csgroup.eu, gerald.schaefer@linux.ibm.com, vgupta@synopsys.com, paul.walmsley@sifive.com References: <1606825169-5229-1-git-send-email-anshuman.khandual@arm.com> <1606825169-5229-2-git-send-email-anshuman.khandual@arm.com> <5d07e798-aa91-ec00-36af-108ff0b19709@arm.com> From: Anshuman Khandual Message-ID: Date: Wed, 2 Dec 2020 17:34:51 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <5d07e798-aa91-ec00-36af-108ff0b19709@arm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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: On 12/2/20 4:46 PM, Steven Price wrote: > On 01/12/2020 12:19, Anshuman Khandual wrote: >> This adds validation tests for dirtiness after write protect conversio= n for >> each page table level. There are two new separate test types involved = here. >> >> The first test ensures that a given page table entry does not become d= irty >> after pxx_wrprotect(). This is important for platforms like arm64 whic= h >> transfers and drops the hardware dirty bit (!PTE_RDONLY) to the softwa= re >> dirty bit while making it an write protected one. This test ensures th= at >> no fresh page table entry could be created with hardware dirty bit set= . >> The second test ensures that a given page table entry always preserve = the >> dirty information across pxx_wrprotect(). >> >> This adds two previously missing PUD level basic tests and while here = fixes >> pxx_wrprotect() related typos in the documentation file. >> >> Cc: Andrew Morton >> Cc: linux-mm@kvack.org >> Cc: linux-kernel@vger.kernel.org >> Suggested-by: Catalin Marinas >> Signed-off-by: Anshuman Khandual >> --- >> =C2=A0 Documentation/vm/arch_pgtable_helpers.rst |=C2=A0 8 ++--- >> =C2=A0 mm/debug_vm_pgtable.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 | 42 +++++++++++++++++++++++ >> =C2=A0 2 files changed, 46 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/vm/arch_pgtable_helpers.rst b/Documentation= /vm/arch_pgtable_helpers.rst >> index f3591ee3aaa8..552567d863b8 100644 >> --- a/Documentation/vm/arch_pgtable_helpers.rst >> +++ b/Documentation/vm/arch_pgtable_helpers.rst >> @@ -50,7 +50,7 @@ PTE Page Table Helpers >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> =C2=A0 | pte_mkwrite=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | Creates a writable PTE=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> -| pte_mkwrprotect=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 | Creates a write protected PTE=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 | >> +| pte_wrprotect=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 | Creates a write protected PTE=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 | >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> =C2=A0 | pte_mkspecial=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 | Creates a special PTE=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> @@ -120,7 +120,7 @@ PMD Page Table Helpers >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> =C2=A0 | pmd_mkwrite=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | Creates a writable PMD=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> -| pmd_mkwrprotect=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 | Creates a write protected PMD=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 | >> +| pmd_wrprotect=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 | Creates a write protected PMD=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 | >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> =C2=A0 | pmd_mkspecial=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 | Creates a special PMD=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> @@ -186,7 +186,7 @@ PUD Page Table Helpers >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> =C2=A0 | pud_mkwrite=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | Creates a writable PUD=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> -| pud_mkwrprotect=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 | Creates a write protected PUD=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 | >> +| pud_wrprotect=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 | Creates a write protected PUD=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 | >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> =C2=A0 | pud_mkdevmap=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 | Creates a ZONE_DEVICE mapped PUD=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 | >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> @@ -224,7 +224,7 @@ HugeTLB Page Table Helpers >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> =C2=A0 | huge_pte_mkwrite=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 | Creates a writable HugeTLB=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 | >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> -| huge_pte_mkwrprotect=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | Creates a writ= e protected HugeTLB=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | >> +| huge_pte_wrprotect=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | Crea= tes a write protected HugeTLB=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> =C2=A0 | huge_ptep_get_and_clear=C2=A0=C2=A0 | Clears a HugeTLB=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | >> =C2=A0 +---------------------------+----------------------------------= ----------------+ >> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >> index c05d9dcf7891..c6fffea54522 100644 >> --- a/mm/debug_vm_pgtable.c >> +++ b/mm/debug_vm_pgtable.c >> @@ -63,6 +63,17 @@ static void __init pte_basic_tests(unsigned long pf= n, pgprot_t prot) >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pte_t pte =3D pfn_pte(pfn, prot); >> =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_debug("Validating PTE basic\n= "); >> + >> +=C2=A0=C2=A0=C2=A0 /* >> +=C2=A0=C2=A0=C2=A0=C2=A0 * This test needs to execute right after the= given page >> +=C2=A0=C2=A0=C2=A0=C2=A0 * table entry is created with pfn_pte() to m= ake sure that >> +=C2=A0=C2=A0=C2=A0=C2=A0 * protection_map[idx] does not have the dirt= y bit enabled >> +=C2=A0=C2=A0=C2=A0=C2=A0 * from the beginning. This is particularly i= mportant for >> +=C2=A0=C2=A0=C2=A0=C2=A0 * platforms like arm64 where (!PTE_RDONLY) i= ndicate dirty >> +=C2=A0=C2=A0=C2=A0=C2=A0 * bit being set. >> +=C2=A0=C2=A0=C2=A0=C2=A0 */ >=20 > Unless I'm seriously mistaken this comment is misleading - the likes of= pte_wrprotect() take the PTE *by value* and return the modified version.= So none of these tests actually modify the variable 'pte'. So there shou= ldn't actually be any restrictions on the ordering. >=20 > Or am I missing something? No, you are right. Seems like I might have confused this for other page table entry altering tests here (via pointers). Although it might still be better to have these tests at the beginning as not to miss a freshly created page table entry. That way the test would not be forgotten and invalidated, in case the subsequent tests in the function change in the future. So yes, the there is no restriction on the ordering here, as I might have mentioned previously. Looking at the comments again. It still seems to be applicable and valid as it does not explicitly refer to the restriction on ordering here. It just stresses on the point that it needs to execute right after creation to test what was in protection_map[idx].