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 30E4DC4345F for ; Fri, 26 Apr 2024 07:44:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8D0E56B0089; Fri, 26 Apr 2024 03:44:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 87FCD6B008A; Fri, 26 Apr 2024 03:44:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 747AB6B008C; Fri, 26 Apr 2024 03:44:00 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 56C216B0089 for ; Fri, 26 Apr 2024 03:44:00 -0400 (EDT) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id A426081317 for ; Fri, 26 Apr 2024 07:43:59 +0000 (UTC) X-FDA: 82050894198.30.5009500 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf19.hostedemail.com (Postfix) with ESMTP id ADC451A0009 for ; Fri, 26 Apr 2024 07:43:57 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=none; spf=pass (imf19.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=1714117438; 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=dSZRfBTBX9KtiJQO+JmW8YIt+sf7q533b7Tab3nEi78=; b=sSkjOp+E2WjKtkKdD3umzxDllIMovnr3d/X+bOVuN1ZRgZmBcsvbH5I6YE8bT6hjHBqicR KEDK5SVJ6rwTx8kzHlQ4KZrkwMPdya/grhBtyTtalOfW958LVf88gZa6NcKAEOS+KuZ4QR gGJ5KgD9VqwztU+kFmV+h+YJcEo17dM= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714117438; a=rsa-sha256; cv=none; b=Il/PAxfhc/QRP/o5pewsaKC1Maz6noUoa4oqQjrM2rHNVtatEyaHXCwPxDNyV6rxISm4LJ 3PpnOV5BbQapygKD2E/Aju+q069JBZODzXlBZtMHVSJV2/HAN0xJt5gkOzeR0P0umLbOj5 Y7fWf1n41yK/Ofwz2QyIhfLURocnIBI= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=none; spf=pass (imf19.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 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 CF2B72F4; Fri, 26 Apr 2024 00:44:24 -0700 (PDT) Received: from [10.57.64.176] (unknown [10.57.64.176]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C5A433F793; Fri, 26 Apr 2024 00:43:55 -0700 (PDT) Message-ID: <87df74b7-2352-4ad6-b9f9-baed9291cbed@arm.com> Date: Fri, 26 Apr 2024 08:43:54 +0100 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-GB To: Anshuman Khandual , Andrew Morton , Zi Yan , "Aneesh Kumar K.V" Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20240425170704.3379492-1-ryan.roberts@arm.com> <39d0236c-4297-4001-bc7e-f516bc674f7b@arm.com> From: Ryan Roberts In-Reply-To: <39d0236c-4297-4001-bc7e-f516bc674f7b@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspam-User: X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: ADC451A0009 X-Stat-Signature: pfmao7f4gq5bwpxnkp98wen1ep83nzi5 X-HE-Tag: 1714117437-41820 X-HE-Meta: U2FsdGVkX1/ZRbB/xeukH2dCV6b+6v+5oQ1MHXkEnqqF1ErnRn3Gav/N+YvGQY3VdM13pxi6eLLUWG9G+CI2JVCN08eFYq4S/CnrBdMzn6x/2fNrSypaNQqbKVoBxphv1gVemYolpulQNKNLPMw1VWIAfIRnD6VZGMdb/FmeM64MYLLysyKnF5cQoOpDT7NCiJ0SdmgQWLoBcv7bEaccZagpepvRjTbI+j3DrmouoUWedqRwSg5/Wta3zWpuccfl7awxWmMUvEfbi+iO0BPz5SaqWivUW7YC/NlWAvXu83ONq0MAm6RSFKvrzH2N+PWh8xu3fB/oTYWY4XqUN6KZfyE9wMFv0I+XHGIpq3sPbt5WmgYhh13Q0KQv9J0vxcoBpnsDjky5sveTZs8/OWSciYjXE4CSyzi08KQPU7abEuuDGf8qFC33PE3FC4MPt0fQefabdsNSHdMnfydijHXc8rxVqJDVkG57xOoUn4HjQ946fsJEZ8Bol/CMLFALRpKtH3F5hblB5kevb36xuHtH4qoTqC81OmtpdfzqnxCfa4IX/knaHItUQWgAtm2B6kr7h0MU4vP0ljIT+x6oM2cZ3EHjsngKjbyZmuZmAZeNpQ9MzLK/O6Fu21S7diKEQnGqo51EkK+YYgs3iZGI7AhOP6ZfEIkSwlirCvBpnlVdnezmSRFs2j//6iMTQR94fxdxbo3vPEtfR4giaLBQktFk6F95FdJr1/DNpv21iLva7FQ2vpjzNEE11+cF5ejbr5axbd5C6KytD0gnp8i4XoIvp6B8CwLq2GR24jW56C36sGUP18U0GAtAUpmX5GsEwocYUo5eYcXpqzpkHsTFaxBFTYkIHKWrwkvBaSHBNn29EIVRX2oMjWUsMobgKB+6xWd1RvN4+IDWykk= 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 26/04/2024 05:19, Anshuman Khandual wrote: > On 4/25/24 22:37, 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. > > pmdp_invalidate() must be called only for present PMD - is this expected > by core MM ? Does this cause any problem otherwise ? I'm saying that only calling pmdp_invalidate() on a pte_present()==true pte is the only semantic that makes sense. And, yes, it causes a problem if called on a pte_present()==false pte - that's exactly what I'm describing in this commit log. To labour the point, this is the logical type hierachy of PTEs (and block-mapped PMDs) as I see it: ---8<---- pte |- present | |- valid | |- invalid | |- not_present |- none |- swap_pte present: All fields must be interpretted the way the HW sees them. e.g. pte_pfn(), pte_write(), pte_dirty(), pte_young(), pte_mkwrite(), pte_mkold() can all be legitimately used to query and modify the pte. valid: The HW may access the pte, interpret the fields and create a TLB entry, etc. invalid: The HW will never access the pte or create a TLB entry for it. not_present: The fields are SW-defined. HW never accesses the PTE. none: Unused; represents a hole swap_pte: Contains a swap entry and swap pte bits. The contained swap entry may 1 of a few different types e.g. actual swap entry, migration entry, hw poison, etc. ---8<---- We test present vs not_present with pte_present() We test none vs swap_pte with pte_none() valid vs invalid is slightly more vague. The core-mm can move a PMD from valid -> invalid by calling pmd_mkinvalid(). But it can't query the state. And it can't do this generically for a PTE. Based on that lot, it makes no sense to me that we should permit calling pmd_mkinvalid() on a non-present pte. Indeed, we don't permit calling pte_mkwrite() etc on a non-present pte. And those functions are not defensive; they don't check that the pte is present before making the change. They just trust that the core-mm will not call them for non-present ptes. The alternative approach would be to make pmdp_invalidate() defensive so that it checks the pmd is present before making any changes. But it doesn't semantically make sense to invalidate a non-present pmd in the first place so why call it under these circumstances? There is also a practical problem in that some arches implement their own pmdp_invalidate() so you would want to make all those defensive too, which would grow the size of the change. > >> >> On arm64 at least, pmd_mkinvalid() will mark the pmd such that any >> future call to pmd_present() will return true. And therefore any > > IIRC the following semantics needs to be followed as expected by core MM. > > ------------------------------------------------------------------------- > | PMD states | pmd_present | pmd_trans_huge | > ------------------------------------------------------------------------- > | Mapped | Yes | Yes | > ------------------------------------------------------------------------- > | Splitting | Yes | Yes | > ------------------------------------------------------------------------- > | Migration/Swap | No | No | > ------------------------------------------------------------------------- Indeed, the problem, as I see it, is if pmd_mkinvalid() is called on a "Migration/Swap" pmd, then a future call to pmd_present() will return Yes, which is clearly wrong. pmd_trans_huge() will also return Yes due to: static inline int pmd_trans_huge(pmd_t pmd) { return pmd_val(pmd) && pmd_present(pmd) && !(pmd_val(pmd) & PMD_TABLE_BIT); } At least this happens for arm64. Although Zi suggests other arches look like they will do this too in the other email. The reason is that arm64's pmd_mkinvalid() unconditionally sets PMD_PRESENT_INVALID (bit 59) and clears PMD_SECT_VALID (bit 0) in the pte. So next time pmd_present() is called it will see PMD_PRESENT_INVALID is set and return true. > > >> 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. > > Could you please explain how bad things might happen ? See 2 places where pmdp_get_lockless() is called in gup.c, without the PTL. These could both return the swap pte for which pmd_mkinvalid() has been called. In both cases, this would lead to the pmd_present() check eroneously returning true, eventually causing incorrect interpretation of the pte fields. e.g.: gup_pmd_range() pmd_t pmd = pmdp_get_lockless(pmdp); gup_huge_pmd(pmd, ...) page = nth_page(pmd_page(orig), (addr & ~PMD_MASK) >> PAGE_SHIFT); page is guff. Let me know what you think! Thanks, Ryan > >> 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 >> >>