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=-10.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,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 31DC8C71156 for ; Mon, 30 Nov 2020 10:58:31 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B39BE207BC for ; Mon, 30 Nov 2020 10:58:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B39BE207BC 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 F37166B0036; Mon, 30 Nov 2020 05:58:27 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id EC0EE8D0003; Mon, 30 Nov 2020 05:58:27 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DB0878D0001; Mon, 30 Nov 2020 05:58:27 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0085.hostedemail.com [216.40.44.85]) by kanga.kvack.org (Postfix) with ESMTP id C0DE96B0036 for ; Mon, 30 Nov 2020 05:58:27 -0500 (EST) Received: from smtpin25.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 8BDA1362D for ; Mon, 30 Nov 2020 10:58:27 +0000 (UTC) X-FDA: 77540785854.25.men88_230aea5273a1 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin25.hostedemail.com (Postfix) with ESMTP id 68F761804E3C4 for ; Mon, 30 Nov 2020 10:58:27 +0000 (UTC) X-HE-Tag: men88_230aea5273a1 X-Filterd-Recvd-Size: 5969 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf08.hostedemail.com (Postfix) with ESMTP for ; Mon, 30 Nov 2020 10:58:26 +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 F163830E; Mon, 30 Nov 2020 02:58:25 -0800 (PST) Received: from [10.163.84.86] (unknown [10.163.84.86]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 696203F66B; Mon, 30 Nov 2020 02:58:23 -0800 (PST) Subject: Re: [PATCH 1/2] mm/debug_vm_pgtable/basic: Add validation for dirtiness after write protect To: Catalin Marinas Cc: Christophe Leroy , linux-mm@kvack.org, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, steven.price@arm.com, gerald.schaefer@linux.ibm.com, vgupta@synopsys.com, paul.walmsley@sifive.com References: <1606453584-15399-1-git-send-email-anshuman.khandual@arm.com> <1606453584-15399-2-git-send-email-anshuman.khandual@arm.com> <20201127094421.GA25070@gaia> <9d9e5c8b-08f6-9ed4-074c-3dafc8fa3717@arm.com> <20201130093841.GA3902@gaia> From: Anshuman Khandual Message-ID: <171a4e71-b1ab-3ff5-7088-54781d960b2a@arm.com> Date: Mon, 30 Nov 2020 16:28:20 +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: <20201130093841.GA3902@gaia> 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 11/30/20 3:08 PM, Catalin Marinas wrote: > On Mon, Nov 30, 2020 at 09:55:00AM +0530, Anshuman Khandual wrote: >> On 11/27/20 3:14 PM, Catalin Marinas wrote: >>> On Fri, Nov 27, 2020 at 09:22:24AM +0100, Christophe Leroy wrote: >>>> Le 27/11/2020 =C3=A0 06:06, Anshuman Khandual a =C3=A9crit=C2=A0: >>>>> This adds validation tests for dirtiness after write protect conver= sion for >>>>> each page table level. This is important for platforms such as arm6= 4 that >>>>> removes the hardware dirty bit while making it an write protected o= ne. This >>>>> also fixes pxx_wrprotect() related typos in the documentation file. >>>> >>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>>>> index c05d9dcf7891..a5be11210597 100644 >>>>> --- a/mm/debug_vm_pgtable.c >>>>> +++ b/mm/debug_vm_pgtable.c >>>>> @@ -70,6 +70,7 @@ static void __init pte_basic_tests(unsigned long = pfn, pgprot_t prot) >>>>> WARN_ON(pte_young(pte_mkold(pte_mkyoung(pte)))); >>>>> WARN_ON(pte_dirty(pte_mkclean(pte_mkdirty(pte)))); >>>>> WARN_ON(pte_write(pte_wrprotect(pte_mkwrite(pte)))); >>>>> + WARN_ON(pte_dirty(pte_wrprotect(pte))); >>>> >>>> Wondering what you are testing here exactly. >>>> >>>> Do you expect that if PTE has the dirty bit, it gets cleared by >>>> pte_wrprotect() ? >>>> >>>> Powerpc doesn't do that, it only clears the RW bit but the dirty >>>> bit remains if it is set, until you call pte_mkclean() explicitely. >>> >>> Arm64 has an unusual way of setting a hardware dirty "bit", it actual= ly >>> clears the PTE_RDONLY bit. The pte_wrprotect() sets the PTE_RDONLY bi= t >>> back and we can lose the dirty information. Will found this and poste= d >>> patches to fix the arm64 pte_wprotect() to set a software PTE_DIRTY i= f >>> !PTE_RDONLY (we do this for ptep_set_wrprotect() already). My concern >>> was that we may inadvertently make a fresh/clean pte dirty with such >>> change, hence the suggestion for the test. >>> >>> That said, I think we also need a test in the other direction, >>> pte_wrprotect() should preserve any dirty information: >>> >>> WARN_ON(!pte_dirty(pte_wrprotect(pte_mkdirty(pte)))); >> >> This seems like a generic enough principle which all platforms should >> adhere to. But the proposed test WARN_ON(pte_dirty(pte_wrprotect(pte))= ) >> might fail on some platforms if the page table entry came in as a dirt= y >> one and pte_wrprotect() is not expected to alter the dirty state. >=20 > Ah, so do we have architectures where entries in protection_map[] are > already dirty? If those are valid, maybe the check should be: Okay, I did not imply that actually. The current position for these new tests in respective pxx_basic_tests() functions is right at the end and hence the pxx might have already gone through some changes from the time it was originally created with pfn_pxx(). The entry here is not starting from the beginning. It is not expected as well, per design. So dirty bit might or might not be there depending on all the previous test sequences leading upto these new ones. IIUC, Christophe mentioned the fact that on platforms like powerpc, dirty bit just remains unchanged during pte_wprotect(). So the current test WARN_ON(pte_dirty(pte_wrprotect(pte))) will not work on powerpc if the previous tests leading upto that point has got the dirty bit set. This is irrespective of how it was created with pfn_pte() from protection_map[] originally at the beginning. >=20 > WARN_ON(!pte_dirty(pte) && pte_dirty(pte_wrprotect(pte))); >=20 >> Instead, should we just add the following two tests, which would ensur= e >> that pte_wrprotect() never alters the dirty state of a page table entr= y. >> >> WARN_ON(!pte_dirty(pte_wrprotect(pte_mkdirty(pte)))); >> WARN_ON(pte_dirty(pte_wrprotect(pte_mkclean(pte)))); >=20 > These should be added as additional tests. However, my initial thought Okay, will add them. > was to check whether pte_wrprotect() on a new pte created from a > protection_map[] entry directly would inadvertently dirty it. On arm64, > that means a protection_map[] entry missing PTE_RDONLY. A pte_mkclean() > would set PTE_RDONLY, so we'd miss such check. >=20 To achieve this, we could move the test right at the beginning just after the pxx gets created from protection_map[], with a comment explaining the rationale.=20