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 E6448C4345F for ; Wed, 1 May 2024 11:38:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 650666B0087; Wed, 1 May 2024 07:38:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5D98A6B0088; Wed, 1 May 2024 07:38:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 47A276B0089; Wed, 1 May 2024 07:38:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 2C4536B0087 for ; Wed, 1 May 2024 07:38:41 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id B2416A0361 for ; Wed, 1 May 2024 11:38:40 +0000 (UTC) X-FDA: 82069629600.13.0F428CF Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf15.hostedemail.com (Postfix) with ESMTP id 05A83A001D for ; Wed, 1 May 2024 11:38:38 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=none; spf=pass (imf15.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=1714563519; 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=fK5csVkZQrJLA4rxF3JAVCWmyPXz8xUtHPDxpmFuvKs=; b=ZcJ1oufylYxYDfaTQ0m2E6af+eJfzLYPTrxbJmqGmlqbonwFXKssQT8cI1vbFU3UX3FJcV u3lt1wIOUiB0noPoLcOo2TVShUp/r89S6BPORUTPOky/p+7H7i21YDcxyd91SqRSVEuIel mq4st8C6acuJPxI2oHqroTyPTxT8syc= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=none; spf=pass (imf15.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=1714563519; a=rsa-sha256; cv=none; b=WyBTRAXAkHP7rK2faLly8yxhQhxCdmIFKg7rXmylaJIgOZNIddCGT49XPbLPF2nh0ZrU+7 nvc8/w37Who980ZGKOnJanHMecF3SsbwieCbDiAb3cFLGpwNobX8Trhmeq764yv/WrokFD W1KzXGwooqHniXC4TQp1A+EYVwWUFuQ= 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 73416339; Wed, 1 May 2024 04:39:04 -0700 (PDT) Received: from [10.57.65.146] (unknown [10.57.65.146]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id ACD493F793; Wed, 1 May 2024 04:38:36 -0700 (PDT) Message-ID: <0dd7827a-6334-439a-8fd0-43c98e6af22b@arm.com> Date: Wed, 1 May 2024 12:38:35 +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 From: Ryan Roberts To: Catalin Marinas , Will Deacon , Mark Rutland , Anshuman Khandual , Andrew Morton , Zi Yan , "Aneesh Kumar K.V" , David Hildenbrand Cc: 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> <9fb15315-6317-4bf4-a736-a8a44288b0c2@arm.com> In-Reply-To: <9fb15315-6317-4bf4-a736-a8a44288b0c2@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Stat-Signature: tzy7bujx5h6o7udnmfk979qpp19ptxqo X-Rspam-User: X-Rspamd-Queue-Id: 05A83A001D X-Rspamd-Server: rspam05 X-HE-Tag: 1714563518-755806 X-HE-Meta: U2FsdGVkX1+jocjUbRkoDIQ08cKnlMs4ZMNzJSNRzyJggIHMFwTYIL/RbZQaZ7afwYv1aQ698RTJRJtAtUPg8pvaGIK73W+FcqlYdvik+RAHQ5x1mb+5qepebKdMfwLPiJqtP+0uql4kyzD/Vdda9xAwgBTHeb1IbeSARh+/lo6gBCRWYAbwMy7m9LhBHyBqRttjhdVLJcSE1n32XnCqecUJCI8U8R75vQSaw8BWfj/qNaiOUCm7SNjIzmM5wbfiKSb6rFZaOn/xQ+/MKzeY0tA2e2uzVcLv2dPS9nV6rpYJQ1lk/JpVLu+XfenfiMOOQl9b6nUEkzjbR5f1mMQvS3CfV40ptY3DcGOzDhJYahR9qhhMWYR6goahIWmCt1ARH3/X0Ht8qXFmeb9dOoHbBl5G4Q5X7XeohvLn2HPuE8+Hjdgr9vQmwnkEDl0hmEIT5IFPqx8ErOSHKVCI93VAajJ4RCGUAIjVstuVBMqYT+CbWEqIgYFozp38wjEJrF31VFkzkyZV1mWnGC/fZzjBOemW59bmC5CB44vBvKNeoAIgMaBOKtvHEaC2gI7d9SK/mV4b1gzUKBi0PBQHECmFKrInRMHmhc0jRk1JrpknlwCf82wHlrfvqWgJrpUhMTupRp6qrHN2NcabGBXR35/LJ/2uJvGUXsmlrhftdyi5zwlW2Mex66mSz0bMFgItzOa3UC/l868/0PirhMhKf9VSSYVLg4yfUnd09UiaRPrT02icD/f3ZRGC/TKkVMad+NkHFw8a+YKM8Yr1vMw3ADIun7846Wt2tfV87vo1b3qecCDJcoK8SyF7AqE5UAlCHFT5cCtnUNZiQfOlsvtZYkqorYH427FIjWTawg6c7sNHW31C66QfSEoShSnHrCMO1Q91Aa8ngNwKKPt6L73ijhll/uyMSyMj6Z3oGN9fnixf1njQqpgNByMfFDU1bajpPbRM6b9or6zHtVPaycWlj14 mDKPL+pt zvd22A/8C+5nDZUU4XmRg2y/PxwVWi66piEQDgHagSzUvunqgcFS5pY3AwV5VzEneE56CnI9LL2VHMGHR9mLpmbSpGHn0+Oj8qwHUn/9ti1AHg+1lQkSU5f/kT7lPW9WpZ94KEuzmztZM+WKN4DvXDemKj9FB5JnNpJd7XFw2+6emF/eEi6gdn46y6NLyTFsm4AwxxCxpE5+HrbTrQEQ5P2rbX6znVu4UptIPyBwtZTXqwbLXksL2eKOOD44cKBaPr6VYa8vVnbtiWD1jcmVlL55LcJtMjagJlj1H0tmggqg+ZsjUhCSsR6AifbJRCEXTVs/E 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: Pulling in David, who may be able to advise... On 01/05/2024 12:35, Ryan Roberts wrote: > Zi Yan, I'm hoping you might have some input on the below... > > > On 30/04/2024 14:31, 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. > > OK the plot thickens; The tests I wrote to check that pmd_mkinvalid() is safe for swap entries fails on x86_64. See below... > >> 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; >> } >> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >> index 65c19025da3d..7e9c387d06b0 100644 >> --- a/mm/debug_vm_pgtable.c >> +++ b/mm/debug_vm_pgtable.c >> @@ -956,6 +956,65 @@ static void __init hugetlb_basic_tests(struct pgtable_debug_args *args) { } >> #endif /* CONFIG_HUGETLB_PAGE */ >> >> #ifdef CONFIG_TRANSPARENT_HUGEPAGE >> +#if !defined(__HAVE_ARCH_PMDP_INVALIDATE) && defined(CONFIG_ARCH_ENABLE_THP_MIGRATION) >> +static void __init swp_pmd_mkinvalid_tests(struct pgtable_debug_args *args) >> +{ > > Printing various values at different locations in this function for debug: > >> + unsigned long max_swap_offset; >> + swp_entry_t swp_set, swp_clear, swp_convert; >> + pmd_t pmd_set, pmd_clear; >> + >> + /* >> + * See generic_max_swapfile_size(): probe the maximum offset, then >> + * create swap entry will all possible bits set and a swap entry will >> + * all bits clear. >> + */ >> + max_swap_offset = swp_offset(pmd_to_swp_entry(swp_entry_to_pmd(swp_entry(0, ~0UL)))); >> + swp_set = swp_entry((1 << MAX_SWAPFILES_SHIFT) - 1, max_swap_offset); >> + swp_clear = swp_entry(0, 0); >> + >> + /* Convert to pmd. */ >> + pmd_set = swp_entry_to_pmd(swp_set); >> + pmd_clear = swp_entry_to_pmd(swp_clear); > > [ 0.702163] debug_vm_pgtable: [swp_pmd_mkinvalid_tests ]: valid: pmd_set=f800000000000000, pmd_clear=7fffffffffffe00 > >> + >> + /* >> + * Sanity check that the pmds are not-present, not-huge and swap entry >> + * is recoverable without corruption. >> + */ >> + WARN_ON(pmd_present(pmd_set)); >> + WARN_ON(pmd_trans_huge(pmd_set)); >> + swp_convert = pmd_to_swp_entry(pmd_set); >> + WARN_ON(swp_type(swp_set) != swp_type(swp_convert)); >> + WARN_ON(swp_offset(swp_set) != swp_offset(swp_convert)); >> + WARN_ON(pmd_present(pmd_clear)); >> + WARN_ON(pmd_trans_huge(pmd_clear)); >> + swp_convert = pmd_to_swp_entry(pmd_clear); >> + WARN_ON(swp_type(swp_clear) != swp_type(swp_convert)); >> + WARN_ON(swp_offset(swp_clear) != swp_offset(swp_convert)); >> + >> + /* Now invalidate the pmd. */ >> + pmd_set = pmd_mkinvalid(pmd_set); >> + pmd_clear = pmd_mkinvalid(pmd_clear); > > [ 0.704452] debug_vm_pgtable: [swp_pmd_mkinvalid_tests ]: invalid: pmd_set=f800000000000000, pmd_clear=7ffffffffe00e00 > >> + >> + /* >> + * Since its a swap pmd, invalidation should effectively be a noop and >> + * the checks we already did should give the same answer. Check the >> + * invalidation didn't corrupt any fields. >> + */ >> + WARN_ON(pmd_present(pmd_set)); >> + WARN_ON(pmd_trans_huge(pmd_set)); >> + swp_convert = pmd_to_swp_entry(pmd_set); > > [ 0.706461] debug_vm_pgtable: [swp_pmd_mkinvalid_tests ]: set: swp=7c03ffffffffffff (1f, 3ffffffffffff), convert=7c03ffffffffffff (1f, 3ffffffffffff) > >> + WARN_ON(swp_type(swp_set) != swp_type(swp_convert)); >> + WARN_ON(swp_offset(swp_set) != swp_offset(swp_convert)); >> + WARN_ON(pmd_present(pmd_clear)); >> + WARN_ON(pmd_trans_huge(pmd_clear)); >> + swp_convert = pmd_to_swp_entry(pmd_clear); > > [ 0.708841] debug_vm_pgtable: [swp_pmd_mkinvalid_tests ]: clear: swp=0 (0, 0), convert=ff8 (0, ff8) > >> + WARN_ON(swp_type(swp_clear) != swp_type(swp_convert)); >> + WARN_ON(swp_offset(swp_clear) != swp_offset(swp_convert)); > > This line fails on x86_64. > > The logs show that the offset is indeed being corrupted by pmd_mkinvalid(); 0 -> 0xff8. > > I think this is due to x86's pmd_mkinvalid() assuming the pmd is present; pmd_flags() and pmd_pfn() do all sorts of weird and wonderful things. > > So does this take us full circle? Are we now back to modifying the core-mm to never call pmd_mkinvalid() on a non-present entry? If so, then I guess we should remove the arm64 fix from for-next/fixes. > >> +} >> +#else >> +static void __init swp_pmd_mkinvalid_tests(struct pgtable_debug_args *args) { } >> +#endif /* !__HAVE_ARCH_PMDP_INVALIDATE && CONFIG_ARCH_ENABLE_THP_MIGRATION */ >> + >> static void __init pmd_thp_tests(struct pgtable_debug_args *args) >> { >> pmd_t pmd; >> @@ -982,6 +1041,8 @@ static void __init pmd_thp_tests(struct pgtable_debug_args *args) >> WARN_ON(!pmd_trans_huge(pmd_mkinvalid(pmd_mkhuge(pmd)))); >> WARN_ON(!pmd_present(pmd_mkinvalid(pmd_mkhuge(pmd)))); >> #endif /* __HAVE_ARCH_PMDP_INVALIDATE */ >> + >> + swp_pmd_mkinvalid_tests(args); >> } >> >> #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD >> -- >> 2.25.1 >> >