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 0A5DCC02182 for ; Thu, 23 Jan 2025 16:17:49 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4C5476B0083; Thu, 23 Jan 2025 11:17:49 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 4733A6B0089; Thu, 23 Jan 2025 11:17:49 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 33AFC280001; Thu, 23 Jan 2025 11:17:49 -0500 (EST) 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 10E2D6B0083 for ; Thu, 23 Jan 2025 11:17:49 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 83DFCC11D2 for ; Thu, 23 Jan 2025 16:17:48 +0000 (UTC) X-FDA: 83039222616.14.B20CA7F Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf14.hostedemail.com (Postfix) with ESMTP id 572B110000E for ; Thu, 23 Jan 2025 16:17:46 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf14.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1737649066; a=rsa-sha256; cv=none; b=KxFJgfmRIMyGa4UeDFdHSH116OvTUslmmgTE4/zx+EMG3Lr9WshFGkKi4VQYwGsPeN8Hx7 Nh4c8NAt57OPRUsHi5o9PKHty2bU3f1SiJ7RDih9jvMuk6JZHzx0sB17RaTpclMaw9gejD 8DC/z6WfW6SY0O82uOkOdpjXGXBpPO4= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf14.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1737649066; 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=LMTcj8LIlGZw9fwh0NBpmiRtqI5CxJkhp2kEyR4Ac1A=; b=NQBISLolK1ew7VYhA3EDZjKF913bByGJ9bpLk556yIt/3h1deCONbOg+2imjfsG3VS/SqW ksz627hgweR8CC0J3s5X+J/x7TSNqlpDohH7+B1JaRq4exS/T9mR0VjMFbe5GClzqNftK3 LCrnrQqJE3StPjAKd5SaUurEIizxNr4= 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 77F841063; Thu, 23 Jan 2025 08:18:13 -0800 (PST) Received: from [10.1.33.169] (XHFQ2J9959.cambridge.arm.com [10.1.33.169]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 862883F5A1; Thu, 23 Jan 2025 08:17:43 -0800 (PST) Message-ID: Date: Thu, 23 Jan 2025 16:17:42 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1 1/2] mm: Clear uffd-wp PTE/PMD state on mremap() Content-Language: en-GB From: Ryan Roberts To: Andrew Morton , Muchun Song , "Liam R. Howlett" , Lorenzo Stoakes , Vlastimil Babka , Jann Horn , Shuah Khan , Peter Xu , David Hildenbrand , =?UTF-8?Q?Miko=C5=82aj_Lenczewski?= , Mark Rutland Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org, stable@vger.kernel.org References: <20250107144755.1871363-1-ryan.roberts@arm.com> <20250107144755.1871363-2-ryan.roberts@arm.com> <850479be-000a-45a7-9669-491d4200a988@arm.com> In-Reply-To: <850479be-000a-45a7-9669-491d4200a988@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Queue-Id: 572B110000E X-Rspamd-Server: rspam10 X-Stat-Signature: 77pmheq1p1txtiy9rfefn9d6yi9qua4k X-HE-Tag: 1737649066-8171 X-HE-Meta: U2FsdGVkX18/M+n+S/tIHEKqfdOsBU6kaVlmXYrAh+QbdvXSWKk2lRDK12EDoScN2kEGB5hchb0leMIJo7sA0ibjoTjSmU1lbuFvsfVxvjDe/Lmo/2h0u68UVhl/JDbOuC2Pxd7wvvyz5z02rMoTKBfvi8SGHZp/yhTvTIJl3eZQXsuRDfZpXUmV6/pFhYxndcChC+FEvMJ4QRI/dcfggcTD2rzxZUhEGSBfM5HUDoD38ecz2w4FOsYohuAJWZv1G5fY2BigBVUDQjJjGZKIDC2ecAwfnbwUCzvn4/JeRi01h50+xiOEzgRq2luzGrfp1NwdM5OuVIUw7kWAe7hH9YKR7hT2FGbY7qk/JEzDcadISivElQxgXQcKXF8GPva9e24iYPvThLrQ6NgCj1mYIEmi2AbPW/EjpVypdCZfbv6ibB/0s5ByLZU4o8alyGhVU9O5BsMQV4BOgRtHAP4YvokVQiJay4VdvF1ejv+BCsSMKeV3HVpu3qdJdtNLDZYQ2IKmNiFd+MHXP0TcLUkUSZcBZvcR6FOABoKA2miymZ2Wr3KivmWnhp/A6sNU19pX5HKmKX3wjRsFASTO5V2gHfb4YZoHVhO3Ks8bgdq+gZqPaahETJFT3ZJ7CtlJFBC77jWce5wIb3NnvCyRP+7YJ5cddIz8MfmUEmWPDxfg7+Xa9OzsIlRTnpKwaABBUapod1tf4Aq14JGnFk3jxwEm7FKOagb2h/iCQewzSmXva8AfSVFcN+G2ZyIvjMax1CTwkbXpFAfbFBuO2Yy2LCjfeoLJ+miaofJpKzVGSmiiMOdYeLG5eGXRPUtCJID2K2ZoOIaSO+TolRhZZgnlk1VBWoKLjQ5XEj0bD6rED5nAA8Gk6oTKBKR/oqcnfr7f440izvCU3xnHMRisoRnSDq5egkhjp8Y50CWoyv9dIhV1alQ+Lwq50vFmNEgjIhgiPXjAV3tGZeLghsmJGrYNP86 IerLwrhd 53N4kmyWkWeh7xWiwLdlQ2j5bm/bcV9BFKAKQnq5OTDRvBUXtmtI5ubn9Y1tB6cUKqDFLJHLBOXrGqtN36OzYu0wEXK5wwx3s1BkMkywWPzLJlfh0EJ35BjoGMZxszXxSNEtBuOv7h883YTcIXiOMbJAs8MvUEsgMpgG2h3g196Iw9t/vq0B9dhqy3iyTX2Zj1dymgonjHBu0zgwPSZijNAcKn5wMScaftHpYQmWVJ1TYkvWvTHb/ueLs/m79hxXEVEeu 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 23/01/2025 14:38, Ryan Roberts wrote: > I think there might be a bug in this after all... > > > On 07/01/2025 14:47, Ryan Roberts wrote: >> When mremap()ing a memory region previously registered with userfaultfd >> as write-protected but without UFFD_FEATURE_EVENT_REMAP, an >> inconsistency in flag clearing leads to a mismatch between the vma flags >> (which have uffd-wp cleared) and the pte/pmd flags (which do not have >> uffd-wp cleared). This mismatch causes a subsequent mprotect(PROT_WRITE) >> to trigger a warning in page_table_check_pte_flags() due to setting the >> pte to writable while uffd-wp is still set. >> >> Fix this by always explicitly clearing the uffd-wp pte/pmd flags on any >> such mremap() so that the values are consistent with the existing >> clearing of VM_UFFD_WP. Be careful to clear the logical flag regardless >> of its physical form; a PTE bit, a swap PTE bit, or a PTE marker. Cover >> PTE, huge PMD and hugetlb paths. >> >> Co-developed-by: Mikołaj Lenczewski >> Signed-off-by: Mikołaj Lenczewski >> Signed-off-by: Ryan Roberts >> Closes: https://lore.kernel.org/linux-mm/810b44a8-d2ae-4107-b665-5a42eae2d948@arm.com/ >> Fixes: 63b2d4174c4a ("userfaultfd: wp: add the writeprotect API to userfaultfd ioctl") >> Cc: stable@vger.kernel.org >> --- >> include/linux/userfaultfd_k.h | 12 ++++++++++++ >> mm/huge_memory.c | 12 ++++++++++++ >> mm/hugetlb.c | 14 +++++++++++++- >> mm/mremap.c | 32 +++++++++++++++++++++++++++++++- >> 4 files changed, 68 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h >> index cb40f1a1d081..75342022d144 100644 >> --- a/include/linux/userfaultfd_k.h >> +++ b/include/linux/userfaultfd_k.h >> @@ -247,6 +247,13 @@ static inline bool vma_can_userfault(struct vm_area_struct *vma, >> vma_is_shmem(vma); >> } >> >> +static inline bool vma_has_uffd_without_event_remap(struct vm_area_struct *vma) >> +{ >> + struct userfaultfd_ctx *uffd_ctx = vma->vm_userfaultfd_ctx.ctx; >> + >> + return uffd_ctx && (uffd_ctx->features & UFFD_FEATURE_EVENT_REMAP) == 0; >> +} >> + >> extern int dup_userfaultfd(struct vm_area_struct *, struct list_head *); >> extern void dup_userfaultfd_complete(struct list_head *); >> void dup_userfaultfd_fail(struct list_head *); >> @@ -402,6 +409,11 @@ static inline bool userfaultfd_wp_async(struct vm_area_struct *vma) >> return false; >> } >> >> +static inline bool vma_has_uffd_without_event_remap(struct vm_area_struct *vma) >> +{ >> + return false; >> +} >> + >> #endif /* CONFIG_USERFAULTFD */ >> >> static inline bool userfaultfd_wp_use_markers(struct vm_area_struct *vma) >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index c89aed1510f1..2654a9548749 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -2212,6 +2212,16 @@ static pmd_t move_soft_dirty_pmd(pmd_t pmd) >> return pmd; >> } >> >> +static pmd_t clear_uffd_wp_pmd(pmd_t pmd) >> +{ >> + if (pmd_present(pmd)) >> + pmd = pmd_clear_uffd_wp(pmd); >> + else if (is_swap_pmd(pmd)) >> + pmd = pmd_swp_clear_uffd_wp(pmd); >> + >> + return pmd; >> +} >> + >> bool move_huge_pmd(struct vm_area_struct *vma, unsigned long old_addr, >> unsigned long new_addr, pmd_t *old_pmd, pmd_t *new_pmd) >> { >> @@ -2250,6 +2260,8 @@ bool move_huge_pmd(struct vm_area_struct *vma, unsigned long old_addr, >> pgtable_trans_huge_deposit(mm, new_pmd, pgtable); >> } >> pmd = move_soft_dirty_pmd(pmd); >> + if (vma_has_uffd_without_event_remap(vma)) >> + pmd = clear_uffd_wp_pmd(pmd); >> set_pmd_at(mm, new_addr, new_pmd, pmd); >> if (force_flush) >> flush_pmd_tlb_range(vma, old_addr, old_addr + PMD_SIZE); >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >> index 354eec6f7e84..cdbc55d5384f 100644 >> --- a/mm/hugetlb.c >> +++ b/mm/hugetlb.c >> @@ -5454,6 +5454,7 @@ static void move_huge_pte(struct vm_area_struct *vma, unsigned long old_addr, >> unsigned long new_addr, pte_t *src_pte, pte_t *dst_pte, >> unsigned long sz) >> { >> + bool need_clear_uffd_wp = vma_has_uffd_without_event_remap(vma); >> struct hstate *h = hstate_vma(vma); >> struct mm_struct *mm = vma->vm_mm; >> spinlock_t *src_ptl, *dst_ptl; >> @@ -5470,7 +5471,18 @@ static void move_huge_pte(struct vm_area_struct *vma, unsigned long old_addr, >> spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING); >> >> pte = huge_ptep_get_and_clear(mm, old_addr, src_pte); >> - set_huge_pte_at(mm, new_addr, dst_pte, pte, sz); >> + >> + if (need_clear_uffd_wp && pte_marker_uffd_wp(pte)) >> + huge_pte_clear(mm, new_addr, dst_pte, sz); > > This is checking if the source huge_pte is a uffd-wp marker and clearing the > destination if so. The destination could have previously held arbitrary valid > mappings, I guess? > > But huge_pte_clear() does not call page_table_check_pte_clear(). So any previous > valid mapping will not have it's page_table_check ref count decremented? > > I think it should be replaced with: > huge_ptep_get_and_clear(mm, new_addr, dst_pte); > > Since there is no huge_ptep_clear(). > > The tests I wrote are always mremapping into PROT_NONE space so they don't hit > this condition. If people agree this is a bug, I'll send out a fix. OK I'm deep in the rabbit hole now; Could anyone clarify the specs for huge_pte_clear() and huge_ptep_get_and_clear()? Specifically, I believe: - huge_pte_clear() can only be called for not-present huge_ptes, because the only way to set a huge_pte is via set_huge_pte_at() and that will always execute the page_table_check_*_set() functions for present huge_ptes. So clearing a present huge_pte using huge_pte_clear(), which does not call page_table_check_*_clear(), would cause counter imbalance. It looks like existing generic-code callers of huge_pte_clear() only do it for non-present huge_ptes. - huge_ptep_get_and_clear() can be used to get-and-clear both present and non-present huge_ptes? There is a single call site in generic-code where this could be called for a non-present huge_pte: move_huge_pte() (and it was there prior to my change). BUT, it looks like the arm64 implementation of huge_ptep_get_and_clear() is not safe to call if the pte being cleared is non-present: pte_t huge_ptep_get_and_clear(struct mm_struct *mm, unsigned long addr, pte_t *ptep) { int ncontig; size_t pgsize; pte_t orig_pte = __ptep_get(ptep); if (!pte_cont(orig_pte)) return __ptep_get_and_clear(mm, addr, ptep); ncontig = find_num_contig(mm, addr, ptep, &pgsize); return get_clear_contig(mm, addr, ptep, pgsize, ncontig); } pte_cont() assumes the pte is present, otherwise it's sampling a random bit that doesn't have the meaning it thinks it has. It is currently relying on that to determine the size of the huge_pte. So either arm64 has a bug or move_huge_pte() has a bug. If huge_ptep_get_and_clear() needs to work for non-present huge_ptes, we will need to pass the huge_pte size into this function. I don't think there is another way to resolve this. Thanks, Ryan > > Thanks, > Ryan >