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 AE7DFE77173 for ; Fri, 6 Dec 2024 04:50:26 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A4C3D6B018D; Thu, 5 Dec 2024 23:50:25 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 9D5926B018E; Thu, 5 Dec 2024 23:50:25 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7D9796B018F; Thu, 5 Dec 2024 23:50:25 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 5A4236B018D for ; Thu, 5 Dec 2024 23:50:25 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id C47A042DC1 for ; Fri, 6 Dec 2024 04:50:24 +0000 (UTC) X-FDA: 82863307842.14.E2FD7B5 Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf12.hostedemail.com (Postfix) with ESMTP id E642E40009 for ; Fri, 6 Dec 2024 04:50:15 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=FJhjBHsy; spf=pass (imf12.hostedemail.com: domain of nathan@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=nathan@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1733460605; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=I9y6UwBigTj+YcM/kCJaYd9WqWmbfsnHZKIjGTq471U=; b=fmr6iPglAVy5MN9AB1vy6V9r0lG2UdJU6VYURJHOLphE1cm10lDp3+T70w9PmcFKorwrCH M0DVecHEzpiDOE2czq5Z7SQNUyyVjxDiMyjoWn8LHeUfDybHknxidi5uwJGSHkxH9w5lLk XWOsPZR2ooq+JgZsF09SWIm1I4Ia9l4= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=FJhjBHsy; spf=pass (imf12.hostedemail.com: domain of nathan@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=nathan@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1733460605; a=rsa-sha256; cv=none; b=SQNLPRRCA58BKsfEi3FpTZZCgEHDOTxRu3Qxsrt3zprz9V3h70R1sZgfsePI7ZD+Vjqo+I Jg8BdGN+bd4s+xVsdnqwnTje8w0H17FkS4Jq/wVfyVQntZZet2eNXgHUjwKPH/jPKP2R4F txkDgUcjmXCpAXbXwM43P4yUoU0fjCM= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 4BAFAA41C03; Fri, 6 Dec 2024 04:48:30 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 350D6C4CED1; Fri, 6 Dec 2024 04:50:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1733460621; bh=azcIaN9xS4OhBh13wKTotBQOrofPdrThVZnKKRGCi5Y=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=FJhjBHsyo2CyO7dZGswIJvUxYIRO9xAsjEWLd98w8UNcfr1Cikneln+LMjOK6JVCI d9trAdJM6ACDpujasfZIeFgJQT80miFednLZt91bNalsYUPFDX8OIrzcJBwE2i/H98 gnHjgjRXaqrhclbMeXBbFbjrGaF4nbPku4GkC1kYn6pfmn7sW3uqPkBwWC71w19c3l vlGsVnpykXr5S0WdkcKe4OvKqdYNUvVpPefJPqO94lZ4YEYE00ece9GDesbXWGm1SL wXIaqz1A5SNiwyZDn9waKgt8O3Go2+rGCaO1uglx3tXdSWiE/tH2Y44z2EG4E8CdJC ftdqdWXplxrCg== Date: Thu, 5 Dec 2024 21:50:19 -0700 From: Nathan Chancellor To: Guillaume Morin , Heiko Carstens , Vasily Gorbik , Alexander Gordeev Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Muchun Song , Andrew Morton , Peter Xu , David Hildenbrand , Eric Hagberg , linux-s390@vger.kernel.org Subject: Re: [PATCH v3] mm/hugetlb: support FOLL_FORCE|FOLL_WRITE Message-ID: <20241206045019.GA2215843@thelio-3990X> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: E642E40009 X-Rspam-User: X-Stat-Signature: 18cd9j7cw4msmwmok68du7chs3fcu47p X-HE-Tag: 1733460615-460976 X-HE-Meta: U2FsdGVkX1/ImLZX+RFklQd/JDZgKWdZC55JoqzrGI4McH4zgdBeC76h2VF1MB6XMxxHc5DdHh3QVw+0ryffTZGdN6lnck3Ilz9ix0Hk5cXbqQmrR/cOXdzpBWfcxOBseCq0vUKVz7ubN8KZi3O6nKVZKvRvtY+nBDkY/q+6Sk/vjKuFMcezV9VpzK8dDksFa/7azJj3BMLVn0kgeZQAxp3NbyzptucJH7yxLy9J5kOt5PWq+41Gx1BGAOhjS07kKfwIUtpA0CEB+fAMJJ/pGAUischRYpmFiVOwJ7P00wmot50F2XuEmqJ2yqajSDW3lbiiJUwlWWY1bhI+Z414PQl0JyBughonycuHS/p7rF9FVyNdAlIbHUBh1CzZDlQJCVD68nIgJsvpklVx4x0fsxgg4vDFMXiIFGg5gGFmzYskHlGIUFFnrwudT8ehBxhrbGosjn7/iUQft+5NKulfa5v0ruhl95FiLQbAW+PYEHmia2REn8dp8K4H1qEB5/9CwVss5RVVqMaRFCS1Kqc3U5Q82JmEQxLiP/LhNz+aSBbjuvmwPMCOBMA8HqkfBhZvkdXT3LK5pr9KU7M0d0QX0AkOtQzhwfyuO15IKG4sExr/4EIP7c5mAensHLZZ/j7ymV2PKgRAmmnCGRq2zWHglrdKvHK9Ah+YidwEHmSiGCQ931YnY7/bQ8zhGaxlnnpnUzKyO9YEznMCAroaghfhh5Y7dfqwGy87Dor1x6/BO4DR9S8vgvWm4Zecd59hKThcioZ9rGL4oJTVS5I4a+fDhwUGhD0axo+zwPEoWLNW1X4i1J8rl3Xn+FuyAae7h4oopj5x3k2eAed95GLPljBesO4uzTL+klmVNPiCxEJaj7PJ6KmnR4syleCRgxgoJqdgAE1SHb3SsLfmcSk/Af+itg2ZTdP8te4umKlX1YWrkqwLiVGsED4KSXaz7mbUxgv8IKLRk/vsNb6gVeBr41X zKkHYUZt C0BlCy3fqXkiiX5+SToHzj6bmGafWd7fN46AW7q4wHoD6m6/H7TXOQVdLXeAq/QD1ow/VGZBfUtthDNUNq+BLsZO97h8xbVwEQYmENeajRCGZIq6PlJiEPaMrfIwS3f0DXVNOK4rtmx94zhpXUiqVOxCEbVPzsFXa6aPdC5xKiHjvBqplMF1lDGzE6y2lm/HnXnxivDsrbh1noJmgSJbq9uNzVzvzCpyi5lMl5jMPINdvPN2ZoqrzgIMHlCqu5BOg/I7WoIVR6pReCwhOBrHbrJje40FcQLwCk/FcFIMvJTuEhK+5n0VOSrf8bCFZM5Rn+CUIamqlAp724OTJI7g/kdpEAeVL8PehBNoWLHDbiMDJGDzRycYaigZBJO34ufGNX5LhAfVZPm/alO7rU0TMx0ruLrKNX6NbeNteyd5K4eyj9545FWtkGhJLOB7w/Eo60roPHMRWaA1qRz9sfCDS+MRq2HN5TSkAGBOw+IJhiRQG+HB5hrPKnyHzIB8yd/xs1FTRTNdvWuLFg9MzT3a07K5iQdMW2magQgSPqjM2RSLrAAOQczcQOXwvaw== 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: Hi Guillaume and s390 folks, On Thu, Dec 05, 2024 at 03:02:26AM +0100, Guillaume Morin wrote: > > Eric reported that PTRACE_POKETEXT fails when applications use hugetlb > for mapping text using huge pages. Before commit 1d8d14641fd9 > ("mm/hugetlb: support write-faults in shared mappings"), PTRACE_POKETEXT > worked by accident, but it was buggy and silently ended up mapping pages > writable into the page tables even though VM_WRITE was not set. > > In general, FOLL_FORCE|FOLL_WRITE does currently not work with hugetlb. > Let's implement FOLL_FORCE|FOLL_WRITE properly for hugetlb, such that > what used to work in the past by accident now properly works, allowing > applications using hugetlb for text etc. to get properly debugged. > > This change might also be required to implement uprobes support for > hugetlb [1]. > > [1] https://lore.kernel.org/lkml/ZiK50qob9yl5e0Xz@bender.morinfr.org/ > > Cc: Muchun Song > Cc: Andrew Morton > Cc: Peter Xu > Cc: David Hildenbrand > Cc: Eric Hagberg > Signed-off-by: Guillaume Morin > --- > Changes in v2: > - Improved commit message > Changes in v3: > - Fix potential unitialized mem access in follow_huge_pud > - define pud_soft_dirty when soft dirty is not enabled > > include/linux/pgtable.h | 5 +++ > mm/gup.c | 99 +++++++++++++++++++++-------------------- > mm/hugetlb.c | 20 +++++---- > 3 files changed, 66 insertions(+), 58 deletions(-) > > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h > index adef9d6e9b1b..9335d7c82d20 100644 > --- a/include/linux/pgtable.h > +++ b/include/linux/pgtable.h > @@ -1422,6 +1422,11 @@ static inline int pmd_soft_dirty(pmd_t pmd) > return 0; > } > > +static inline int pud_soft_dirty(pud_t pud) > +{ > + return 0; > +} > + > static inline pte_t pte_mksoft_dirty(pte_t pte) > { > return pte; > diff --git a/mm/gup.c b/mm/gup.c > index 746070a1d8bf..cc3eae458013 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -587,6 +587,33 @@ static struct folio *try_grab_folio_fast(struct page *page, int refs, > } > #endif /* CONFIG_HAVE_GUP_FAST */ > > +/* Common code for can_follow_write_* */ > +static inline bool can_follow_write_common(struct page *page, > + struct vm_area_struct *vma, unsigned int flags) > +{ > + /* Maybe FOLL_FORCE is set to override it? */ > + if (!(flags & FOLL_FORCE)) > + return false; > + > + /* But FOLL_FORCE has no effect on shared mappings */ > + if (vma->vm_flags & (VM_MAYSHARE | VM_SHARED)) > + return false; > + > + /* ... or read-only private ones */ > + if (!(vma->vm_flags & VM_MAYWRITE)) > + return false; > + > + /* ... or already writable ones that just need to take a write fault */ > + if (vma->vm_flags & VM_WRITE) > + return false; > + > + /* > + * See can_change_pte_writable(): we broke COW and could map the page > + * writable if we have an exclusive anonymous page ... > + */ > + return page && PageAnon(page) && PageAnonExclusive(page); > +} > + > static struct page *no_page_table(struct vm_area_struct *vma, > unsigned int flags, unsigned long address) > { > @@ -613,6 +640,22 @@ static struct page *no_page_table(struct vm_area_struct *vma, > } > > #ifdef CONFIG_PGTABLE_HAS_HUGE_LEAVES > +/* FOLL_FORCE can write to even unwritable PUDs in COW mappings. */ > +static inline bool can_follow_write_pud(pud_t pud, struct page *page, > + struct vm_area_struct *vma, > + unsigned int flags) > +{ > + /* If the pud is writable, we can write to the page. */ > + if (pud_write(pud)) > + return true; > + > + if (!can_follow_write_common(page, vma, flags)) > + return false; > + > + /* ... and a write-fault isn't required for other reasons. */ > + return !vma_soft_dirty_enabled(vma) || pud_soft_dirty(pud); This looks to be one of the first uses of pud_soft_dirty() in a generic part of the tree from what I can tell, which shows that s390 is lacking it despite setting CONFIG_HAVE_ARCH_SOFT_DIRTY: $ make -skj"$(nproc)" ARCH=s390 CROSS_COMPILE=s390-linux- mrproper defconfig mm/gup.o mm/gup.c: In function 'can_follow_write_pud': mm/gup.c:665:48: error: implicit declaration of function 'pud_soft_dirty'; did you mean 'pmd_soft_dirty'? [-Wimplicit-function-declaration] 665 | return !vma_soft_dirty_enabled(vma) || pud_soft_dirty(pud); | ^~~~~~~~~~~~~~ | pmd_soft_dirty Is this expected? Cheers, Nathan > +} > + > static struct page *follow_huge_pud(struct vm_area_struct *vma, > unsigned long addr, pud_t *pudp, > int flags, struct follow_page_context *ctx) > @@ -625,13 +668,16 @@ static struct page *follow_huge_pud(struct vm_area_struct *vma, > > assert_spin_locked(pud_lockptr(mm, pudp)); > > - if ((flags & FOLL_WRITE) && !pud_write(pud)) > + pfn += (addr & ~PUD_MASK) >> PAGE_SHIFT; > + page = pfn_to_page(pfn); > + > + if ((flags & FOLL_WRITE) && > + !can_follow_write_pud(pud, page, vma, flags)) > return NULL; > > if (!pud_present(pud)) > return NULL; > > - pfn += (addr & ~PUD_MASK) >> PAGE_SHIFT; > > if (IS_ENABLED(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD) && > pud_devmap(pud)) { > @@ -653,8 +699,6 @@ static struct page *follow_huge_pud(struct vm_area_struct *vma, > return ERR_PTR(-EFAULT); > } > > - page = pfn_to_page(pfn); > - > if (!pud_devmap(pud) && !pud_write(pud) && > gup_must_unshare(vma, flags, page)) > return ERR_PTR(-EMLINK); > @@ -677,27 +721,7 @@ static inline bool can_follow_write_pmd(pmd_t pmd, struct page *page, > if (pmd_write(pmd)) > return true; > > - /* Maybe FOLL_FORCE is set to override it? */ > - if (!(flags & FOLL_FORCE)) > - return false; > - > - /* But FOLL_FORCE has no effect on shared mappings */ > - if (vma->vm_flags & (VM_MAYSHARE | VM_SHARED)) > - return false; > - > - /* ... or read-only private ones */ > - if (!(vma->vm_flags & VM_MAYWRITE)) > - return false; > - > - /* ... or already writable ones that just need to take a write fault */ > - if (vma->vm_flags & VM_WRITE) > - return false; > - > - /* > - * See can_change_pte_writable(): we broke COW and could map the page > - * writable if we have an exclusive anonymous page ... > - */ > - if (!page || !PageAnon(page) || !PageAnonExclusive(page)) > + if (!can_follow_write_common(page, vma, flags)) > return false; > > /* ... and a write-fault isn't required for other reasons. */ > @@ -798,27 +822,7 @@ static inline bool can_follow_write_pte(pte_t pte, struct page *page, > if (pte_write(pte)) > return true; > > - /* Maybe FOLL_FORCE is set to override it? */ > - if (!(flags & FOLL_FORCE)) > - return false; > - > - /* But FOLL_FORCE has no effect on shared mappings */ > - if (vma->vm_flags & (VM_MAYSHARE | VM_SHARED)) > - return false; > - > - /* ... or read-only private ones */ > - if (!(vma->vm_flags & VM_MAYWRITE)) > - return false; > - > - /* ... or already writable ones that just need to take a write fault */ > - if (vma->vm_flags & VM_WRITE) > - return false; > - > - /* > - * See can_change_pte_writable(): we broke COW and could map the page > - * writable if we have an exclusive anonymous page ... > - */ > - if (!page || !PageAnon(page) || !PageAnonExclusive(page)) > + if (!can_follow_write_common(page, vma, flags)) > return false; > > /* ... and a write-fault isn't required for other reasons. */ > @@ -1285,9 +1289,6 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) > if (!(vm_flags & VM_WRITE) || (vm_flags & VM_SHADOW_STACK)) { > if (!(gup_flags & FOLL_FORCE)) > return -EFAULT; > - /* hugetlb does not support FOLL_FORCE|FOLL_WRITE. */ > - if (is_vm_hugetlb_page(vma)) > - return -EFAULT; > /* > * We used to let the write,force case do COW in a > * VM_MAYWRITE VM_SHARED !VM_WRITE vma, so ptrace could > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index ea2ed8e301ef..52517b7ce308 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -5169,6 +5169,13 @@ static void set_huge_ptep_writable(struct vm_area_struct *vma, > update_mmu_cache(vma, address, ptep); > } > > +static void set_huge_ptep_maybe_writable(struct vm_area_struct *vma, > + unsigned long address, pte_t *ptep) > +{ > + if (vma->vm_flags & VM_WRITE) > + set_huge_ptep_writable(vma, address, ptep); > +} > + > bool is_hugetlb_entry_migration(pte_t pte) > { > swp_entry_t swp; > @@ -5802,13 +5809,6 @@ static vm_fault_t hugetlb_wp(struct folio *pagecache_folio, > if (!unshare && huge_pte_uffd_wp(pte)) > return 0; > > - /* > - * hugetlb does not support FOLL_FORCE-style write faults that keep the > - * PTE mapped R/O such as maybe_mkwrite() would do. > - */ > - if (WARN_ON_ONCE(!unshare && !(vma->vm_flags & VM_WRITE))) > - return VM_FAULT_SIGSEGV; > - > /* Let's take out MAP_SHARED mappings first. */ > if (vma->vm_flags & VM_MAYSHARE) { > set_huge_ptep_writable(vma, vmf->address, vmf->pte); > @@ -5837,7 +5837,8 @@ static vm_fault_t hugetlb_wp(struct folio *pagecache_folio, > SetPageAnonExclusive(&old_folio->page); > } > if (likely(!unshare)) > - set_huge_ptep_writable(vma, vmf->address, vmf->pte); > + set_huge_ptep_maybe_writable(vma, vmf->address, > + vmf->pte); > > delayacct_wpcopy_end(); > return 0; > @@ -5943,7 +5944,8 @@ static vm_fault_t hugetlb_wp(struct folio *pagecache_folio, > spin_lock(vmf->ptl); > vmf->pte = hugetlb_walk(vma, vmf->address, huge_page_size(h)); > if (likely(vmf->pte && pte_same(huge_ptep_get(mm, vmf->address, vmf->pte), pte))) { > - pte_t newpte = make_huge_pte(vma, &new_folio->page, !unshare); > + const bool writable = !unshare && (vma->vm_flags & VM_WRITE); > + pte_t newpte = make_huge_pte(vma, &new_folio->page, writable); > > /* Break COW or unshare */ > huge_ptep_clear_flush(vma, vmf->address, vmf->pte); > -- > 2.39.1 > > -- > Guillaume Morin