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 E6DE9C4345F for ; Fri, 26 Apr 2024 04:50:13 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 53EBC6B0083; Fri, 26 Apr 2024 00:50:13 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4ECE46B0085; Fri, 26 Apr 2024 00:50:13 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3B4146B0088; Fri, 26 Apr 2024 00:50:13 -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 1DBFC6B0083 for ; Fri, 26 Apr 2024 00:50:13 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id C43F280604 for ; Fri, 26 Apr 2024 04:50:12 +0000 (UTC) X-FDA: 82050456264.27.17F0665 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf06.hostedemail.com (Postfix) with ESMTP id BCE2818001E for ; Fri, 26 Apr 2024 04:50:10 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf06.hostedemail.com: domain of anshuman.khandual@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=anshuman.khandual@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714107011; 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=Lan0u+/KzK9CCrdnDHUz7uHmYzT5UUtdvyneLZkUcN4=; b=nKsw3XD7gxHBKVrh2iJgxitNfjhVDfkIhbJ4QGnxllxiAMRI1ng9dvHmka9Z/81/Z5LcOG cQFBtloqIYdgiWK1zGmuqiKrt6N2f+K0Mfamd/NJkPv/7QO/Fhor+IM1iIlVlJHWJ/AXXH JdPC6tc8D02S0BxihvQYmuyAvQvn95Y= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf06.hostedemail.com: domain of anshuman.khandual@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=anshuman.khandual@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714107011; a=rsa-sha256; cv=none; b=GFeruypKY7jvCGmBGgU01nbUmpcj0VSrtIjoxaqp9QseNUM16NMV2sP/iWztyz1m9ZCrsF tHu5RphlsG5q4TK6HI9pIarsR6fdyLbfdQHZHVw2z8QNS0Bznu7thIXq7oUP4wphgtmEZc Qt7OwofMOamRl5P5WJT+LkKjZFCmVFY= 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 DD3FB2F; Thu, 25 Apr 2024 21:50:37 -0700 (PDT) Received: from [10.163.32.46] (unknown [10.163.32.46]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F1A6F3F64C; Thu, 25 Apr 2024 21:50:06 -0700 (PDT) Message-ID: Date: Fri, 26 Apr 2024 10:20:03 +0530 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1] mm: Fix race between __split_huge_pmd_locked() and GUP-fast Content-Language: en-US To: Zi Yan , Ryan Roberts Cc: Andrew Morton , "Aneesh Kumar K.V" , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Huacai Chen References: <20240425170704.3379492-1-ryan.roberts@arm.com> <922F6794-1C32-4862-8A67-90D4F7DBC474@nvidia.com> From: Anshuman Khandual In-Reply-To: <922F6794-1C32-4862-8A67-90D4F7DBC474@nvidia.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: BCE2818001E X-Rspam-User: X-Stat-Signature: gpcczhkupcdx1u94j5j5oqf39xc5jsmf X-HE-Tag: 1714107010-987974 X-HE-Meta: U2FsdGVkX18s+zMp9alPPqTRS1j5DhSFTCy9ZNV+4aPu8nKg2pWrz1qgjNZXtS8HWV5wE1nF97xb9d+7PQJrrAC4MncehqnDnDThaX+7qA/dO14BYEQWB1s5hLYtU+3H5deWWVxMAOhuj1lnpGrqYF2zRlf4blWuWJCUlWyFyTh/fppF2NFd9ZUfW3YX3/lmGox3gnDFwLtaIN9ctUHAg9Rh3on1DBZGgAHmz+JAKOeA50M+Po0SU/PFCV+AMJrZZkHiljxMDlGRSlUh3hbewljMGhB8I064c8iyP3ZCsYETsdeeRjpXvMKxmJTiJJbST7sk5vcNyCy2pF/qTfaLVrFt+VkC/vdtii0wsSFRYBn6W5PfDcaOyJwfLsRQMBq83PXs9YtGnKayB3WR0AIhuYzhUl0K9J60u2RUfwYQFTLWDsiuyWTvq9SIiqao0G/Rl1VR2U8mplzwNyUB2yfSvXwzrpgSuqLhq7e6seHN5JaXmQalRBvU92xTWfXYEtFSMinz6jqqJiuGi1Jxa3V66iZBSdwo1P9cyUNoAPKO7gbsg+LQ/3gRbH1XqOu/83NmYOWdviQp/TinQqFO2eeQbrGUSZ44ARo4P4ES9D1nHRHYZIKWD5RtYvrYYzXJetaxjhcEnkviq9xluTwFUDETTJOD7mGPJhKbdfd63OCo0invyEocyBWS8Fqiirn77bpZWnbrCovFj/1BukZ0X64KoefSMvmugLPTs1Y7DwEyYmQc3gJCBKBWyh3fw8RcZ6CI3QLiJ24v/hPmSLeyIypCEYNIWCFgZUlgWNRVTBNF0jPfXDIcLxLAT8RDCqJgT7KTeyq3irYvSJ8Iqm/SdKBL8Y2cgFC2EE9HyBviZ7MwhkTQsHcuETI/53jK8noJLNe7n1M0O1eIPAb/fVbaOaPQg/aEUeN6e6FTFoYkaCJ+bK684hI5qxpOB/x6nFtRKnkASTV40GIQMtrxr8QOPWI reqXnl01 S8OShaWMkGZEjIsdZZIRHRfBXuxN6HZqdMJALs+tnR0prBS3PZUIQSOzfPRSnCPq9qQPYk/hNiYBjrRDWaFU+BHtFXgkorE2VYoPC24W4wnsnZ+W1PzMyYtPJiYmjQ33bgGO+ubPje/yJ98dg52Op6a8nDfz05gJJxmEsHhfbGVujS8wfFS0KJPYvIUGXG5ffPXJpclv2xItAq1a29yw8cpLMHsykbaDZhtsLVLBEEY+deEBPqYB6RZK/SsKl75OuCKFQ0krIiD2kGWoUfivHqbfOKSws1LGrIKZ9TekOHL1na9qKYrpvQ/Bpzg== 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 4/26/24 00:28, Zi Yan wrote: > +Anshuman, who changed pmd_present() semantics. See: > https://lore.kernel.org/all/1599627183-14453-2-git-send-email-anshuman.khandual@arm.com/ and commit b65399f6111b ("arm64/mm: Change > THP helpers to comply with generic MM semantics") > > On 25 Apr 2024, at 13:07, 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. This is a problem for the migration entry >> case because pmd_mkinvalid(), called by pmdp_invalidate() must only be >> called for a present pmd. >> >> On arm64 at least, pmd_mkinvalid() will mark the pmd such that any >> future call to pmd_present() will return true. And therefore any > > But pmd_mkinvalid() on x86 does not behave so. Maybe we should fix > pmd_mkinvalid() on arm64 by not setting PMD_PRESENT_INVALID when the > entry is invalid already. And add a test in mm/debug_vm_pgtable.c. > > I notice that x86, risc-v, mips behave the same. loongarch also > has _PAGE_PRESENT_INVALID bit set during pmd_mkinvalid(), but its > pmd_present() makes sure _PAGE_HUEG is set before checks _PAGE_PRESENT_INVALID. > So it is not a problem for loongarch. Add Huacai to confirm this. > > Maybe pmd_present() on arm64 can do that too? pmd_present() should return true even for a splitting PMD which is not currently mapped. IIRC in all other architectures, there is a distinct identification bit for huge page which stays back, even when the entry becomes unmapped. That bit helps pmd_present() return true, during PMD splitting process. But on arm64 platform #define PTE_VALID (_AT(pteval_t, 1) << 0) #define PMD_SECT_VALID (_AT(pmdval_t, 1) << 0) #define PMD_TABLE_BIT (_AT(pmdval_t, 1) << 1) When the PMD entry becomes unmapped, PTE_VALID gets cleared, so does the PMD_SECT_VALID, thus erasing its identity as a huge mapping. A software bit PMD_PRESENT_INVALID was added which helps in preserving that cleared huge page mapping identity once it becomes unmapped. > >> lockless pgtable walker could see the migration entry pmd in this state >> and start interpretting the fields as if it were present, leading to >> BadThings (TM). GUP-fast appears to be one such lockless pgtable walker. >> I suspect the same is possible on other architectures. >> >> Fix this by only calling pmdp_invalidate() for a present pmd. And for >> good measure let's add a warning to the generic implementation of >> pmdp_invalidate(). I've manually reviewed all other >> pmdp_invalidate[_ad]() call sites and believe all others to be >> conformant. >> >> This is a theoretical bug found during code review. I don't have any >> test case to trigger it in practice. >> >> Fixes: 84c3fc4e9c56 ("mm: thp: check pmd migration entry in common path") >> Signed-off-by: Ryan Roberts >> --- >> >> Applies on top of v6.9-rc5. Passes all the mm selftests on arm64. >> >> Thanks, >> Ryan >> >> >> mm/huge_memory.c | 5 +++-- >> mm/pgtable-generic.c | 2 ++ >> 2 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index 89f58c7603b2..80939ad00718 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -2513,12 +2513,12 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, >> * for this pmd), then we flush the SMP TLB and finally we write the >> * non-huge version of the pmd entry with pmd_populate. >> */ >> - old_pmd = pmdp_invalidate(vma, haddr, pmd); >> >> - pmd_migration = is_pmd_migration_entry(old_pmd); >> + pmd_migration = is_pmd_migration_entry(*pmd); >> if (unlikely(pmd_migration)) { >> swp_entry_t entry; >> >> + old_pmd = *pmd; >> entry = pmd_to_swp_entry(old_pmd); >> page = pfn_swap_entry_to_page(entry); >> write = is_writable_migration_entry(entry); >> @@ -2529,6 +2529,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, >> soft_dirty = pmd_swp_soft_dirty(old_pmd); >> uffd_wp = pmd_swp_uffd_wp(old_pmd); >> } else { >> + old_pmd = pmdp_invalidate(vma, haddr, pmd); >> page = pmd_page(old_pmd); >> folio = page_folio(page); >> if (pmd_dirty(old_pmd)) { >> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c >> index 4fcd959dcc4d..74e34ea90656 100644 >> --- a/mm/pgtable-generic.c >> +++ b/mm/pgtable-generic.c >> @@ -198,6 +198,7 @@ pgtable_t pgtable_trans_huge_withdraw(struct mm_struct *mm, pmd_t *pmdp) >> pmd_t pmdp_invalidate(struct vm_area_struct *vma, unsigned long address, >> pmd_t *pmdp) >> { >> + VM_WARN_ON(!pmd_present(*pmdp)); >> pmd_t old = pmdp_establish(vma, address, pmdp, pmd_mkinvalid(*pmdp)); >> flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE); >> return old; >> @@ -208,6 +209,7 @@ pmd_t pmdp_invalidate(struct vm_area_struct *vma, unsigned long address, >> pmd_t pmdp_invalidate_ad(struct vm_area_struct *vma, unsigned long address, >> pmd_t *pmdp) >> { >> + VM_WARN_ON(!pmd_present(*pmdp)); return pmdp_invalidate(vma, address, pmdp); >> } >> #endif >> -- >> 2.25.1 > > > -- > Best Regards, > Yan, Zi