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 6DAECC4345F for ; Tue, 30 Apr 2024 14:04:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F0E166B00C8; Tue, 30 Apr 2024 10:04:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id EBE3B6B00C9; Tue, 30 Apr 2024 10:04:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D86286B00CA; Tue, 30 Apr 2024 10:04:39 -0400 (EDT) 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 BB78C6B00C8 for ; Tue, 30 Apr 2024 10:04:39 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 67360A05D7 for ; Tue, 30 Apr 2024 14:04:39 +0000 (UTC) X-FDA: 82066368678.17.B74105C Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf21.hostedemail.com (Postfix) with ESMTP id 056731C000B for ; Tue, 30 Apr 2024 14:04:35 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=none; spf=pass (imf21.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714485876; 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; bh=HL0YKRA/vdJhdntVZlq9QbCb8WfesyMDCwwpKopGRo0=; b=ki9dHVGIyaEsf0ntMkt4B+rJ81PuGl4Ivh2dGCAjvrTqDTzJYHJI8mPa5eeCiNad0VgrHk C9GZJNa7Oj2ApnuWerv/TAuBL8V1Iu7iPu4tYnzoT70AFGngVMAHLZlJKy0JoekRXscXcb bHsfLm8yAz+AFFk2l4v6D8kUNxgOxyM= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=none; spf=pass (imf21.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714485876; a=rsa-sha256; cv=none; b=zm65N0sMlnXzsnPngmCEEPztlfOOHpIxd6AxBZw/T3+oT31Ybg+uCWq9ncjmfxlvuWEDn5 sIzYyO69v6o/Go5KkxWpwdkSH//EooJLMjLjM8bDrNEwJ6qM9D0NEU+/kQwbZ6UfYYRc5E By09E9rQRi9JFRPHH8T5NUnISYcXn4Y= 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 837FC2F4; Tue, 30 Apr 2024 07:05:01 -0700 (PDT) Received: from [10.1.38.140] (XHFQ2J9959.cambridge.arm.com [10.1.38.140]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B13F13F793; Tue, 30 Apr 2024 07:04:33 -0700 (PDT) Message-ID: <41a83b7a-17e0-469d-bec4-10ebfff4ef57@arm.com> Date: Tue, 30 Apr 2024 15:04:32 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] arm64/mm: pmd_mkinvalid() must handle swap pmds Content-Language: en-GB To: Will Deacon Cc: Catalin Marinas , Mark Rutland , Anshuman Khandual , Andrew Morton , Zi Yan , "Aneesh Kumar K.V" , linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org References: <20240430133138.732088-1-ryan.roberts@arm.com> <20240430135534.GA14069@willie-the-truck> From: Ryan Roberts In-Reply-To: <20240430135534.GA14069@willie-the-truck> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 056731C000B X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: deazn5d8b56bamg5wq6ur6e7zehjm4cy X-HE-Tag: 1714485875-795599 X-HE-Meta: U2FsdGVkX19DECq3GHWH3mRjOWO0wIN9NAgYK1vBg5c2FErvDpgB6Kp3EvqZPMwMn/zIEugjkTTwcCPfixuLh3r0TSC4NrSPsySZvzynlVJS+jjMiW4XwL2ZJdr4hTojOLRtNjYhXfAlmOIRPlBEcJiL9aTvQ85T5pCA4C/Wn6MOU7H/KbP0ANx9wtEX8j/mJeGFwdgGx/IUv5MkF6NOXO0F+XF4yOHmMwawfwnZexExn/6sjQqUl9iRh3W/4GoVOshodRtcV55iYiHy/4VobqpwVA2cN3qJ5vAm9LAo7hA/NfnnwU2BFIIf57k3zxcW8+nnNE/CgT4zYCoXmEvCmno4ByBUYBkkkDGXEk+glT0rj/DgUonFOJcgtL7yE/O7SvjAaDY5u03etXg7FnvvT6sTEO/2jTdmzUhPYqnIE+i+Gjap1iqOWDbQHIc5PRvWWF5bDpgzakc3s+jLuL3Ghr0OZXJrdy0VLM6yymJPc3PVV4f0ACwiM321cmlxgemgUvv6154htgE2TNjksiqdTwVdFiKnvZUS7KdjQ5TgQ9r94+/vdFrySwL56ppQYF9SiamPWIgIY8lpY8pBCBo7htNR1/ZR46YjB+A+VR1v1QKwzqL1+rlLFWdYN8xeRdNz/iPKIG5LUvi9JH7YkIYMoE0+tZKYUBCxH7yOEM1eiWU1Z0RRVOP867rFExRjaLiGlGXSXsIFmoZOS0w4LiRQqzbLb+GD9kbhJtsuCcfAtxrTri7kaXrP/LFO6r1iWhfGLYGUGaAxZ3zQMmw4D6BpEMGdSIX3dDsDvZJzU/RmUn4q3jYLgxq35wpWRN2dqjuwbkxOxHPW4GMiwiEGDuVDgMMjUEzX5McsNAtvPCWduArUOKMcR68yQ/uenH0S5CJR+1sXeH5JHzoliKqEM4NlHavZ8/Em33kaknY8eTak4CthgQ3/y6g6jnx5C6A6sqsjrDPG/Gzi6HuskORSHKw V1c/DbDQ PIZ1dcND4bERXDrOV4P4+rlJxEN+LczrsM5Sd8h8Xe172hQqChzQu1EmDTqiiKq6Zk40DqZUgxhnT9u4JmjLu4ZMLJJ66GS0xqfFYqEderuuTfiu4l5LIa0Ys4N9IrMpFfDhiwLuGrCzXPPOmXD7JJjJxhVJYDn9Cim0Da6HrV3tTAQcpusW+VJjg1reETtkXXOyhNFN9+/+Q8s7QyBEX59KW4MoEFuA6SYx1KkvJ/7Y3SsRN+bh9fjshIFTsqb+HqIzdrJah13Oy2DH2uuEm4bNQlrg2OxaZnagiH8p5JNoM3H8sY+/WZK5A/g== 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 30/04/2024 14:55, Will Deacon wrote: > On Tue, Apr 30, 2024 at 02:31:38PM +0100, Ryan Roberts wrote: >> __split_huge_pmd_locked() can be called for a present THP, devmap or >> (non-present) migration entry. It calls pmdp_invalidate() >> unconditionally on the pmdp and only determines if it is present or not >> based on the returned old pmd. >> >> But arm64's pmd_mkinvalid(), called by pmdp_invalidate(), >> unconditionally sets the PMD_PRESENT_INVALID flag, which causes future >> pmd_present() calls to return true - even for a swap pmd. Therefore any >> lockless pgtable walker could see the migration entry pmd in this state >> and start interpretting the fields (e.g. pmd_pfn()) as if it were >> present, leading to BadThings (TM). GUP-fast appears to be one such >> lockless pgtable walker. >> >> While the obvious fix is for core-mm to avoid such calls for non-present >> pmds (pmdp_invalidate() will also issue TLBI which is not necessary for >> this case either), all other arches that implement pmd_mkinvalid() do it >> in such a way that it is robust to being called with a non-present pmd. >> So it is simpler and safer to make arm64 robust too. This approach means >> we can even add tests to debug_vm_pgtable.c to validate the required >> behaviour. >> >> This is a theoretical bug found during code review. I don't have any >> test case to trigger it in practice. >> >> Cc: stable@vger.kernel.org >> Fixes: 53fa117bb33c ("arm64/mm: Enable THP migration") >> Signed-off-by: Ryan Roberts >> --- >> >> Hi all, >> >> v1 of this fix [1] took the approach of fixing core-mm to never call >> pmdp_invalidate() on a non-present pmd. But Zi Yan highlighted that only arm64 >> suffers this problem; all other arches are robust. So his suggestion was to >> instead make arm64 robust in the same way and add tests to validate it. Despite >> my stated reservations in the context of the v1 discussion, having thought on it >> for a bit, I now agree with Zi Yan. Hence this post. >> >> Andrew has v1 in mm-unstable at the moment, so probably the best thing to do is >> remove it from there and have this go in through the arm64 tree? Assuming there >> is agreement that this approach is right one. >> >> This applies on top of v6.9-rc5. Passes all the mm selftests on arm64. >> >> [1] https://lore.kernel.org/linux-mm/20240425170704.3379492-1-ryan.roberts@arm.com/ >> >> Thanks, >> Ryan >> >> >> arch/arm64/include/asm/pgtable.h | 12 +++++-- >> mm/debug_vm_pgtable.c | 61 ++++++++++++++++++++++++++++++++ >> 2 files changed, 71 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h >> index afdd56d26ad7..7d580271a46d 100644 >> --- a/arch/arm64/include/asm/pgtable.h >> +++ b/arch/arm64/include/asm/pgtable.h >> @@ -511,8 +511,16 @@ static inline int pmd_trans_huge(pmd_t pmd) >> >> static inline pmd_t pmd_mkinvalid(pmd_t pmd) >> { >> - pmd = set_pmd_bit(pmd, __pgprot(PMD_PRESENT_INVALID)); >> - pmd = clear_pmd_bit(pmd, __pgprot(PMD_SECT_VALID)); >> + /* >> + * If not valid then either we are already present-invalid or we are >> + * not-present (i.e. none or swap entry). We must not convert >> + * not-present to present-invalid. Unbelievably, the core-mm may call >> + * pmd_mkinvalid() for a swap entry and all other arches can handle it. >> + */ >> + if (pmd_valid(pmd)) { >> + pmd = set_pmd_bit(pmd, __pgprot(PMD_PRESENT_INVALID)); >> + pmd = clear_pmd_bit(pmd, __pgprot(PMD_SECT_VALID)); >> + } >> >> return pmd; >> } > > Acked-by: Will Deacon Thanks > > But it might be worth splitting the tests from the fix to make backporting > easier. Yes good point. I'll leave this hanging for today to see if any more comments come in, and will re-post tomorrow as 2 patches. I assume we need to go fast to catch 6.9. > > Catalin -- I assume you'll pick this up, but please shout if you want me > to take it instead. > > Will