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 735F2C4345F for ; Mon, 29 Apr 2024 03:36:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7527C6B007B; Sun, 28 Apr 2024 23:36:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 701806B0083; Sun, 28 Apr 2024 23:36:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 617356B0085; Sun, 28 Apr 2024 23:36:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 44FE16B007B for ; Sun, 28 Apr 2024 23:36:31 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id ADA9A41722 for ; Mon, 29 Apr 2024 03:36:30 +0000 (UTC) X-FDA: 82061156940.19.04E6105 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf11.hostedemail.com (Postfix) with ESMTP id AA89040011 for ; Mon, 29 Apr 2024 03:36:28 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf11.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=1714361789; 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=AHfVp5cixgnGxnNfe0UZJaSHyRHBx4LbgXQYY/QC6Sw=; b=ATPS8ovTyXVvA5BnbcCveBNin36+0GEQnlKh1dSRVw6LyTog9K4L695Gd9xEUAasqgc35/ 4FG5MIaW8pZgT28L2V7riIOMAoziCv3n+EHurcirBTyXsJ6nfkrhPralJl6dnCbg0CuPFI GeuDZT5zISDgnqYd4XATBEQMkJjLQMo= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf11.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=1714361789; a=rsa-sha256; cv=none; b=y6nqmJbhKmzuFmc93DATExWlwuX6mTW3XrFy+wQtaAtLQm2MvrCTvHeiqGF/jn+aXLUotT V1v1Eydx8DghitVfzhBWR+fNrsDlE6T5JRMmUpDCyzsX5YGUSQP+4CfRs1ks9n2k82fr/L 7gq6hpjjkrN3w52TPLkFKymvD/LSpbk= 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 CF93E2F4; Sun, 28 Apr 2024 20:36:54 -0700 (PDT) Received: from [10.162.42.72] (a077893.blr.arm.com [10.162.42.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3FB573F73F; Sun, 28 Apr 2024 20:36:24 -0700 (PDT) Message-ID: <34a9ec83-40ab-4028-8ee3-27438665155f@arm.com> Date: Mon, 29 Apr 2024 09:06:22 +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 Cc: Ryan Roberts , 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> <683CC7EC-3FAB-4C63-92F2-636C4DBE7729@nvidia.com> From: Anshuman Khandual In-Reply-To: <683CC7EC-3FAB-4C63-92F2-636C4DBE7729@nvidia.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam01 X-Stat-Signature: kr969rordb4easfnxa887bcbxtkeap7f X-Rspam-User: X-Rspamd-Queue-Id: AA89040011 X-HE-Tag: 1714361788-491724 X-HE-Meta: U2FsdGVkX1+6HK0zrRqDDtrgf2rzORp8GpyHecUEXB6pXbynsVM8WJD9fWHLpc4dfLP5KL9V4paPBnCaQEJ0iJpSbcPaUms2xf8FwkVX8tx8W4uYCgOy8qufiqdzuuf8Kl9RkmdJMXZc7vEd454KELYTU88Y7mibXO/o8r2kkVNmpd9f8o3tFG78orGpkCIz2mN+NJXbjpDIya+f9NmEOEOrMcwvokj4vkluyD9bEmw0kz3FcPGQYEdGL9zz2DJzx6oon61W9d0AYQm7j45zPxDTEPvMZJ0wd+KQsjzodnwnKCu8S58RIfKzHFsRoJPrhvno+HeqE5nrkwDImt8jpi7I6LF30D7NHNMyr8zw2Qv4t+wg1Xgv6iwzVLM55csVagH7TsttyAB/BkrGHobXzbDgllvR5Y00hjOplOLp4h15GmtpHamSiPaCwJfIvPYbUcwXUj2K03IUAbP/Pu9fBzpE6P+yK8oTiEHFibKEgMYevGF5Kpg6UZcZU3auKynmm6rMmtCGPEEmIji0Z2OpbCJiOWMyWJplOoXmti8S9p5b1fIoBswTw1V0oFbW34ahvkXh1E9FsWDPbTzFcJu/RJqkqUuTujxae3tMgWKtvOnnSFtmgScH4bVQXDHvhMIg93djN+yom/nHEw58iRyyNBWlCQCQz6ib6IIVPN+OKxrJqYVSKtL0bxdOszWPWofN5BOpvFIROEjSi8hghcGQ5hYLgW0bozkYXwEJy1cyxlti+95Roi7V/s4eH1kA7q4Tn4LI68BiE3xy4/GutmhYk03VF1fY6rjfWqOIHVHQ/lNQhNi4Cd8n3tMqlqk/hqvjvklCCEUiyI+8QQyONOCfI7VkDUEGx9CD5IDAx2toUDaYxpYCljx440BD2aZvBr1z5rFzPVS3CZhxNZtSsOJ49WemXrU6Sw9M3ed1rr3cz9At6h3RGYOZyYNU2DKyl34v/YG+cLjgeddXWH02UtF 7cQ0r0+m MO1/rLUG1FJS8nvD1aElzldVkuwi/7VOO4GypXSCUOdue3z6Rj4RfVXeFbiCeroHJrYiskJMti/4KmtG9zSkFYZ2QU4f9Mmd7sTtR9vb9UrS1ftLKmHjFPjPB4OXSh2xW8W7KRW3gDuJDSmgMUTfpg5h37cBgOkE84QgioCsSVto85LiHlnszUmG5T6eeOLStbv9DImDmwOewb1pPki/Bp3a2YEWryvQhkeZwuKQfli3OnLONKDOsbZCmhnIjlywq92ajOYlPkA9fC0QgYMMXueCD9TBZqu52gnAaqLoLih+S3ITx7H01WOGZKA== 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 20:03, Zi Yan wrote: > > > -- > Best Regards, > Yan, Zi > > On 26 Apr 2024, at 0:50, Anshuman Khandual wrote: > >> 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. > > OK. PMD_SECT_VALID is just a different name of PTE_VALID. I wonder Non PTE Level descriptor BIT[1:0] = X0 Invalid entry = 01 Block entries (Huge Pages) = 11 Table entries PTE Level descriptor BIT[1:0] = X0 Invalid entry = 01 Reserved invalid entry = 11 Page entries Although PTE_VALID and PMD_SECT_VALID share the same bit position, the huge page also requires the table bit to be cleared. > if ~PMD_TABLE_BIT can be used as _PAGE_HUGE to indicate it is a huge page > PMD, since PMD_TABLE_BIT is unset for PMD huge page already, for swap > entry, since PMD_SECT_VALID is unset, PMD_TABLE_BIT is ignored. But it > will require PTE and PMD have different swap entry encoding on arm64. > It might not be worth the effort. Right and also depending on just clearing of table bit for huge page mapping might be problematic in subtle ways. > >>> >>>> 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