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 B15BCC4345F for ; Thu, 2 May 2024 07:34:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 13F8F6B0083; Thu, 2 May 2024 03:34:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0F04C6B0085; Thu, 2 May 2024 03:34:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id ED3CD6B0087; Thu, 2 May 2024 03:34:05 -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 CC9F76B0083 for ; Thu, 2 May 2024 03:34:05 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 2C016A1AD9 for ; Thu, 2 May 2024 07:34:00 +0000 (UTC) X-FDA: 82072641840.21.795A38C Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf23.hostedemail.com (Postfix) with ESMTP id 74B7F140007 for ; Thu, 2 May 2024 07:33:57 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=none; dmarc=temperror reason="SPF/DKIM temp error" header.from=arm.com (policy=temperror); spf=temperror (imf23.hostedemail.com: error in processing during lookup of ryan.roberts@arm.com: DNS error) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714635237; 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=PSQ1QqKxSzU/IOZAYHfmlAzp+gCi8thDTSSqmhPcPc0=; b=BUxL2KWgtgNPkvgK1Ocku5Ex5cnHmmnSBZN0fIPed9Jem+BFkTrTDj3Iv0kSgwHGm+0Ggw E6U2Lsfen3KcM+/PJP5v4sjWEnTqFwzhkZ/PiOje/x/u9EBHvcVaReeMj6t27czc+7KUs5 sAANRhlqsl8hjjq+kcuSeXV2/dy1OWc= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=none; dmarc=temperror reason="SPF/DKIM temp error" header.from=arm.com (policy=temperror); spf=temperror (imf23.hostedemail.com: error in processing during lookup of ryan.roberts@arm.com: DNS error) smtp.mailfrom=ryan.roberts@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714635237; a=rsa-sha256; cv=none; b=CnMwLcLsOE63fmaNr4s2ZTHjIBFdy+Jb7sMCNHLplZI2B0hiib4/qnb1PMERJKYa3wpsTu CDeYQc/UQkM0T+6Wrh6Mj1iVRHv8z2s/fRdlVhbChu86WyGwD+vOv+q+06Glzt1OPH4NCv lq5NkKKhhhwaK3cnJAx2u4Not7PBXgY= 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 2DDB52F4; Thu, 2 May 2024 00:34:22 -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 388F43F71E; Thu, 2 May 2024 00:33:53 -0700 (PDT) Message-ID: <2f95dac3-51cc-474d-8ef0-7af9dd09784b@arm.com> Date: Thu, 2 May 2024 08:33:51 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3] mm: Fix race between __split_huge_pmd_locked() and GUP-fast Content-Language: en-GB To: Zi Yan Cc: Andrew Morton , Catalin Marinas , Will Deacon , Mark Rutland , Anshuman Khandual , "Aneesh Kumar K.V" , Jonathan Corbet , Nicholas Piggin , Christophe Leroy , "Naveen N. Rao" , Christian Borntraeger , Sven Schnelle , "David S. Miller" , Andreas Larsson , Dave Hansen , Andy Lutomirski , Peter Zijlstra , Thomas Gleixner , Ingo Molnar , Borislav Petkov , linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org References: <20240501143310.1381675-1-ryan.roberts@arm.com> <4992C22D-D7B7-4B6F-8A50-E084163BEF42@nvidia.com> From: Ryan Roberts In-Reply-To: <4992C22D-D7B7-4B6F-8A50-E084163BEF42@nvidia.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam01 X-Stat-Signature: nb7smco3xusfbegff4sxmjb8eptbind4 X-Rspam-User: X-Rspamd-Queue-Id: 74B7F140007 X-HE-Tag: 1714635237-121472 X-HE-Meta: U2FsdGVkX1/YrDWFunkNp/witZD8zCEgf5/C+mZ//IEmIpyQpHLVt5yPnqk4JSu7V03Wmuni8HNZX5aV3siEdTQTmcNcpmjwR+6dUo+38Kt1+eshxSmo6qH/8AmMHDsG4YYYtHjbmpAAsLqJYTM6StCtaYoJbl0wSRqFtDB7iFUgNe5b2NggUzC8c0Teyw6DThegRcmNOoUboBBikiWiq7klha2pCnniyb9rp6Evg5JIkqpr6wgZ5Fk9dbu3gHv3NHWYpMLM6pByJ/vzmvgqZ8ZBXC0tsnrs3S/1hk8+Lvw0akvGI1zbLjn9vNmgUZjwoLXVYtLfRMdQ6DQDNvzpon8Em5+JA+9LCdlwTnF70mkvMdE+Lu35sYDPVwbqh5C5h+7Zqy6a5KhbR0biXs/QP4B6WNC3++0r6HqM9u3diXhLoQgbyFpwCAE4PoEV/e2FN4lsYwlJJ5usDGlorqFTwaIif+O1G/kbyQt0OKcW/EjOonDsew0QrxEfHgc1+QmwskU7SiLwfmUkBixCgwJqRmkU+bJjtrOhXfYN+0ezskIS47QcSs7aY8D97NI+5LyWIiObLcnHp+Ie2L2X/GdT0X8FqhAvzSCsHOQnSZT6H3OvbM5I3CQfmEd/i0rYjAazh8nl22XKps6C7+PEfLl0Q5aVfmMkkLSANfBVp61wYbJuRl/L/LAYU8yM2e+uNFp0p1p+fgOI+ADyQPHwjVGSOH0WNEcGj9uL5iftGFikgovaUtHvsdQu+Y7JdI0b4oAn2QBFT2G6hb0PAt6UVLlJVT8thiCIdW4w0Ur4nUaFgPyHcRJOjGGxaUzkrvg4iKoHVMEyT16zbyEmN1Wes3ZJP7KaXXzrd09kMEHgUULoCWbnUIOJ/3upt4g0jOuqZnUeimkfvZAQ2/BAha/NFLYoNufHkt2zWqsPaC5fJ81e/3BOZwei2ioKy3u9fr46XVWYfGEFBNK1x4Bb2/lixQG cCN2sdWJ HnOYVNj19clD7cng6Way2MrAwjYsjPNNMbCzJ2qF70Xm8tkOPDi2TayCFuxaEYnEw2IVZdac0CcIE/EzmzTNK24YigOOGPW1V7Tat49C8fOWtDLHOO2u3S8+s5rBskOr4ZCuYDpDgLPvsV4IcOd7Ime4AsZjTq99+fV3YJ8QNabqvzhKpVMa7w0/OlcAb5SQmTAnqrr59xA5psm5G7q6/0jPSBas1gVwfjNHCaQkfHJGzcqZtP9nnYBpsp2UCNTAR610+Yn1hpmldnZETSJOqiGY//rt+qftXYgif81OAFEHNhJzp0BUBGC6OKc6HSvlI/z1UIjLpoFAOmNHuowF/rhZyHiN2PblL1EExhibuhFiCLlQ= 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 02/05/2024 02:27, Zi Yan wrote: > On 1 May 2024, at 10:33, 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 >> 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. >> >> x86 does not suffer the above problem, but instead pmd_mkinvalid() will >> corrupt the offset field of the swap entry within the swap pte. See link >> below for discussion of that problem. >> >> Fix all of this by only calling pmdp_invalidate() for a present pmd. And >> for good measure let's add a warning to all implementations of >> pmdp_invalidate[_ad](). 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. >> >> Cc: stable@vger.kernel.org >> Link: https://lore.kernel.org/all/0dd7827a-6334-439a-8fd0-43c98e6af22b@arm.com/ >> Fixes: 84c3fc4e9c56 ("mm: thp: check pmd migration entry in common path") >> Signed-off-by: Ryan Roberts >> --- >> >> Right v3; this goes back to the original approach in v1 to fix core-mm rather >> than push the fix into arm64, since we discovered that x86 can't handle >> pmd_mkinvalid() being called for non-present pmds either. >> >> I'm pulling in more arch maintainers because this version adds some warnings in >> arch code to help spot incorrect usage. >> >> Although Catalin had already accepted v2 (fixing arm64) [2] into for-next/fixes, >> he's agreed to either remove or revert it. >> >> >> Changes since v1 [1] >> ==================== >> >> - Improve pmdp_mkinvalid() docs to make it clear it can only be called for >> present pmd (per JohnH, Zi Yan) >> - Added warnings to arch overrides of pmdp_invalidate[_ad]() (per Zi Yan) >> - Moved comment next to new location of pmpd_invalidate() (per Zi Yan) >> >> >> [1] https://lore.kernel.org/linux-mm/20240425170704.3379492-1-ryan.roberts@arm.com/ >> [2] https://lore.kernel.org/all/20240430133138.732088-1-ryan.roberts@arm.com/ >> >> Thanks, >> Ryan >> >> >> Documentation/mm/arch_pgtable_helpers.rst | 6 ++- >> arch/powerpc/mm/book3s64/pgtable.c | 1 + >> arch/s390/include/asm/pgtable.h | 4 +- >> arch/sparc/mm/tlb.c | 1 + >> arch/x86/mm/pgtable.c | 2 + >> mm/huge_memory.c | 49 ++++++++++++----------- >> mm/pgtable-generic.c | 2 + >> 7 files changed, 39 insertions(+), 26 deletions(-) > > The changes in Documentation/mm and mm/* look good to me. Thanks. > Reviewed-by: Zi Yan Thanks! > > I wonder if making Documentation/mm and mm/* changes in a separate patch > would be better, since you will not need acks from arch maintainers and > get the patch in quicker. Yeah maybe - I considered that, but then decided I'm literally just adding a debug warning to the arch code so it shouldn't be too controversial. Perhaps wait a few days for acks and if nothing turns up then I'll re-post with it split? > > > -- > Best Regards, > Yan, Zi