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 03BDEC4345F for ; Fri, 26 Apr 2024 04:20:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4B9496B0087; Fri, 26 Apr 2024 00:20:03 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 43FF26B0088; Fri, 26 Apr 2024 00:20:03 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2E0816B008A; Fri, 26 Apr 2024 00:20:03 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 0DD4B6B0087 for ; Fri, 26 Apr 2024 00:20:03 -0400 (EDT) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id AA8D81406AC for ; Fri, 26 Apr 2024 04:20:02 +0000 (UTC) X-FDA: 82050380244.09.608D49E Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf03.hostedemail.com (Postfix) with ESMTP id B3CDE20013 for ; Fri, 26 Apr 2024 04:20:00 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf03.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=1714105201; 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=MMfJKghXzbfob4CMRd21FaaBsVXga9ceWpPyAs6Xh9E=; b=3Y1tpVFAruEatlaplbx04FGXuQv6o2X8cc7vWU1ICVbNNq6w+5ezxMv1s8HugVARdnucmJ /2eH3mGJfb5kiSx5X1g5kcgAHKBb5nNf/soFjtZRoEEKo+AkPnK7iAXhrujjAFMLCMKKX8 HG2pLgr+zIkOwT1lBpAkHR75BJhBQQ8= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf03.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=1714105201; a=rsa-sha256; cv=none; b=1yOedqEgzBxs4rXaA8Wi2l++G/1PGSfUuVe4A+eSGKJ9kCTIMoB5qtGzZivNJO19dyZYHg FenrD0xh9oSIVubMcptR04B7xrk4oiICKlCRi14cSt2nwFcpAZFTRSxJLwZZkhthZACu28 TmWgRHukeKIpKfbRt8ABUcWldLdpKrE= 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 C14C11007; Thu, 25 Apr 2024 21:20:27 -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 67D2A3F64C; Thu, 25 Apr 2024 21:19:57 -0700 (PDT) Message-ID: <39d0236c-4297-4001-bc7e-f516bc674f7b@arm.com> Date: Fri, 26 Apr 2024 09:49:53 +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: Ryan Roberts , 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> From: Anshuman Khandual In-Reply-To: <20240425170704.3379492-1-ryan.roberts@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Stat-Signature: ko87pdoeuu93wmuf1dddwaiepd5drqjw X-Rspamd-Queue-Id: B3CDE20013 X-Rspamd-Server: rspam10 X-Rspam-User: X-HE-Tag: 1714105200-87209 X-HE-Meta: U2FsdGVkX19mRJzeSJGKXlpQYVQHBLwMdClrjkutctC62+rkkICAV1vHQXjZJAjGvwKWYkK5rPumLsBfZTYgB2ZBZQi9RGUtUvFgqNoiT4SBXmCN7rOTSKW8z/GkoyaPvNUwfamXMFrmISNsLzPqRPnsCIwjSOmSdDVgV4UeY97t2b57DCURRRqTbPYlfDz/FqP+M6zECfjSDhzKi6l9BZ6zmHBIxk+5oJ8Cxq2QIzXNr8FeDJ8FTR3BAHr6jdFHVkh4i150ILTn3xjAUTV78tv0OMh54XcwjWgmj2G2o+ozduAPajTCwa917YtieaCZZGWgMRz5jRjWNYe5U5gCPjGK2RRbSUiG0JWC+Gwo7tHLGFxQaKEzMMhYmimEE1Hg8gb1+r+QfFlH0EHPhzmrxFp18rN03dHPaacQ+0tBTI6+bryUefRGSrubRPoQUl+i+o91SKiKC36qzp3/L86lPcgy09R27YhYsGjIbOsj18poGF9DdL25RV5FaRtmW18HLoyC3NRx7xvfA1kmHBSV2WyH0PfXTylghsClq0ew79SGG5/ZHN0aIogIHS5hXjZFZSb9t6+NkiXhRkARv6/LzKAg091hn2fRmScuJOXJvtpUVJC67K/TRPccR9ZfAorLB5TXzP0bLX3qR4HObkohwhoRLK4GUrcG9V46oEl2QZOLzB+VqutYow9XNJvfOBqKcgva1vQSv54EBiJbIexg2oRCWDgt9WFYBLtmlDW0QZZB4DkpNcEdRjIm8kBzrcIo5m90/H4pA8rpQjH5HEK6iAfq9XTI6LocIHx6sKKSMKk03VdiLhNGqrg/cVKE1Z1gqoaA3vEH55nbhMF6Kmw/FfEws6jpq3VhD+PEl/U6c3iwMHftViSamlVLW+JibjNN/7A/rc4vCyxgUiWe2N2+JoCvMoUYvOQawt4chH2g23uVf4fagjr/M3R/9VxYZ3sW713BD/OnRHb66Ofs+Gb 5cTdc/XO mo+3/FikLflPIWSOSJR80hsyZ2rYRpZoaItRAloe/4NjB6y66EQWLkLOsmPkZ+Sf7GMdpnbEG6rwzst1jUk8xhX/LYgKxMKMcdxBbX5gBw/X723PKHYu4NmtFbSYFqLBxi25HuPRN+SO64uF5Z1ipzkvSVaTZ1SzanvLobvlriV7TppNcwqxPwgcbZXtKMK52ZgL9mkL/nDc8CD5XFRlR0QIaSfdouya/vxKiWftBIpBUbLH+2u7T2wIlpQ== 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/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 ? > > 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 | ------------------------------------------------------------------------- > 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 ? > 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 > >