Received-SPF: pass (google.com: domain of linux-kernel+bounces-160105-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 26 Apr 2024 09:54:33 -0400
From: Peter Xu <peterx@redhat.com>
To: Ryan Roberts <ryan.roberts@arm.com>
Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Joey Gouly <joey.gouly@arm.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Anshuman Khandual <anshuman.khandual@arm.com>,
	David Hildenbrand <david@redhat.com>,
	Mike Rapoport <rppt@linux.ibm.com>,
	Shivansh Vij <shivanshvij@outlook.com>,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/2] arm64/mm: Add uffd write-protect support
Message-ID: <ZiuyGXt0XWwRgFh9@x1n>
References: <20240424111017.3160195-1-ryan.roberts@arm.com>
 <20240424111017.3160195-3-ryan.roberts@arm.com>
 <Zijzrje2FDXsSojP@x1n>
 <e0fdf7b9-39d6-4eb3-b740-9c110b75182f@arm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <e0fdf7b9-39d6-4eb3-b740-9c110b75182f@arm.com>

On Fri, Apr 26, 2024 at 02:17:41PM +0100, Ryan Roberts wrote:
> + Muhammad Usama Anjum <usama.anjum@collabora.com>
> 
> Hi Peter, Muhammad,
> 
> 
> On 24/04/2024 12:57, Peter Xu wrote:
> > Hi, Ryan,
> > 
> > On Wed, Apr 24, 2024 at 12:10:17PM +0100, Ryan Roberts wrote:
> >> Let's use the newly-free PTE SW bit (58) to add support for uffd-wp.
> >>
> >> The standard handlers are implemented for set/test/clear for both pte
> >> and pmd. Additionally we must also track the uffd-wp state as a pte swp
> >> bit, so use a free swap entry pte bit (3).
> >>
> >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> > 
> > Looks all sane here from userfault perspective, just one comment below.
> > 
> >> ---
> >>  arch/arm64/Kconfig                    |  1 +
> >>  arch/arm64/include/asm/pgtable-prot.h |  8 ++++
> >>  arch/arm64/include/asm/pgtable.h      | 55 +++++++++++++++++++++++++++
> >>  3 files changed, 64 insertions(+)
> >>
> >> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> >> index 7b11c98b3e84..763e221f2169 100644
> >> --- a/arch/arm64/Kconfig
> >> +++ b/arch/arm64/Kconfig
> >> @@ -255,6 +255,7 @@ config ARM64
> >>  	select SYSCTL_EXCEPTION_TRACE
> >>  	select THREAD_INFO_IN_TASK
> >>  	select HAVE_ARCH_USERFAULTFD_MINOR if USERFAULTFD
> >> +	select HAVE_ARCH_USERFAULTFD_WP if USERFAULTFD
> >>  	select TRACE_IRQFLAGS_SUPPORT
> >>  	select TRACE_IRQFLAGS_NMI_SUPPORT
> >>  	select HAVE_SOFTIRQ_ON_OWN_STACK
> >> diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
> >> index ef952d69fd04..f1e1f6306e03 100644
> >> --- a/arch/arm64/include/asm/pgtable-prot.h
> >> +++ b/arch/arm64/include/asm/pgtable-prot.h
> >> @@ -20,6 +20,14 @@
> >>  #define PTE_DEVMAP		(_AT(pteval_t, 1) << 57)
> >>  #define PTE_PROT_NONE		(PTE_UXN)		 /* Reuse PTE_UXN; only when !PTE_VALID */
> >>  
> >> +#ifdef CONFIG_HAVE_ARCH_USERFAULTFD_WP
> >> +#define PTE_UFFD_WP		(_AT(pteval_t, 1) << 58) /* uffd-wp tracking */
> >> +#define PTE_SWP_UFFD_WP		(_AT(pteval_t, 1) << 3)	 /* only for swp ptes */
> 
> I've just noticed code in task_mmu.c:
> 
> static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
> 				  unsigned long end, struct mm_walk *walk)
> {
> 	...
> 
> 	if (!p->arg.category_anyof_mask && !p->arg.category_inverted &&
> 	    p->arg.category_mask == PAGE_IS_WRITTEN &&
> 	    p->arg.return_mask == PAGE_IS_WRITTEN) {
> 		for (addr = start; addr < end; pte++, addr += PAGE_SIZE) {
> 			unsigned long next = addr + PAGE_SIZE;
> 
> 			if (pte_uffd_wp(ptep_get(pte))) <<<<<<
> 				continue;
> 
> 			...
> 		}
> 	}
> }
> 
> As far as I can see, you don't know that the pte is present when you do this. So
> does this imply that the UFFD-WP bit is expected to be in the same position for
> both present ptes and swap ptes? I had assumed pte_uffd_wp() was for present
> ptes and pte_swp_uffd_wp() was for swap ptes.
> 
> As you can see, the way I've implemented this for arm64 the bit is in a
> different position for these 2 cases. I've just done a slightly different
> implementation that changes the first patch in this series quite a bit and a
> bunch of pagemap_ioctl mm kselftests are now failing. I think this is the root
> cause, but haven't proven it definitively yet.
> 
> I'm inclined towords thinking the above is a bug and should be fixed so that I
> can store the bit in different places. What do you think?

Yep I agree.

Even on x86_64 they should be defined differently.  It looks like some
sheer luck the test constantly pass on x86 even if it checked the wrong one.

Worth checking all the relevant paths in the pagemap code to make sure it's
checked, e.g. I also see one fast path above this chunk of code which looks
like to have the same issue.

Thanks,

-- 
Peter Xu