From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 6F7BA145355 for ; Mon, 15 Dec 2025 10:57:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765796257; cv=none; b=THcsM8JxCgImf9E8hL0+5GOV9XZ/XuWNJNs5ymShQRC6OlZxhNbJW0XK8R3ix2qN3t03EWTcdvBtZWv6x32tseIN07ZubELiBiT6AFnNbuXAHYw66tCyQm0WWPgTpSFGfm2teabxvyNhVILckEWZvYRy+MWPEvwIbJeoUIHn6jA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765796257; c=relaxed/simple; bh=DU3liR9/4Pl2rHpvlyLxejLPX+p0J8N4kA4isZAAwkA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=SZnwjgtU3gJ9aNeRFIS7pdaFfphSnmHL4A1aGjeVmsoGauyG+ne7x0wyJ67JlWSdfK+kkbsBLxo072XMtOXhwm+mW8iqz6H8XNi9xAyV39VsZJpg7I7j1enUY40wB8Q9e7nm6BzOeRrgL8q7RgQE3PgKmoi/4NQ46BshFpDYa48= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com 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 89F03497; Mon, 15 Dec 2025 02:57:27 -0800 (PST) Received: from [10.1.30.155] (XHFQ2J9959.cambridge.arm.com [10.1.30.155]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BC2553F73B; Mon, 15 Dec 2025 02:57:32 -0800 (PST) Message-ID: <6c76b8e6-3e39-41a0-a4fe-9012c3eb8446@arm.com> Date: Mon, 15 Dec 2025 10:57:31 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/2] mm/madvise: Use set_pte() to write page tables Content-Language: en-GB To: Lorenzo Stoakes Cc: Samuel Holland , Andrew Morton , "Liam R . Howlett" , David Hildenbrand , Vlastimil Babka , Jann Horn , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Anshuman Khandual , Gavin Shan , Zi Yan References: <20251211081117.1126521-1-samuel.holland@sifive.com> <20251211081117.1126521-3-samuel.holland@sifive.com> <1b7e682e-86a4-4573-b423-65f9755f71ea@arm.com> <5daa9569-a3a3-40ad-86d4-ad47080fa5aa@lucifer.local> From: Ryan Roberts In-Reply-To: <5daa9569-a3a3-40ad-86d4-ad47080fa5aa@lucifer.local> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 15/12/2025 10:37, Lorenzo Stoakes wrote: > On Thu, Dec 11, 2025 at 09:43:58AM +0000, Ryan Roberts wrote: >> On 11/12/2025 08:11, Samuel Holland wrote: >>> Generic code must always use the architecture-provided helper function >>> to write page tables. >>> >>> Fixes: 662df3e5c376 ("mm: madvise: implement lightweight guard page mechanism") >>> Signed-off-by: Samuel Holland >>> --- >>> >>> mm/madvise.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/mm/madvise.c b/mm/madvise.c >>> index b617b1be0f535..4da9c32f8738a 100644 >>> --- a/mm/madvise.c >>> +++ b/mm/madvise.c >>> @@ -1114,7 +1114,7 @@ static int guard_install_set_pte(unsigned long addr, unsigned long next, >>> unsigned long *nr_pages = (unsigned long *)walk->private; >>> >>> /* Simply install a PTE marker, this causes segfault on access. */ >>> - *ptep = make_pte_marker(PTE_MARKER_GUARD); >>> + set_pte(ptep, make_pte_marker(PTE_MARKER_GUARD)); >> >> No! As I explained in my response on the other thread (which you linked in the >> cover letter), it is correct as is and should not be changed to set_pte(). > > Yup agreed, esp. given this is my code :) > > Also some arches don't define set_pte()... it seems set_xxx() functions not > really intended to be used outside of arch code - see > e.g. https://elixir.bootlin.com/linux/v6.18.1/A/ident/set_pte > >> >> Copy/pasting my explanation: >> >> | I tried "fixing" this before. But it's correct as is. ptep is pointing to a >> | value on the stack. See [2]. >> | >> | https://lore.kernel.org/linux-mm/2308a4d0-273e-4cf8-9c9f-3008c42b6d18@arm.com/ >> >> If you go look at where this function is called from, you'll see that it's a >> pointer to a stack variable: >> >> >> ---8<--- >> static int walk_pte_range_inner(pte_t *pte, unsigned long addr, >> unsigned long end, struct mm_walk *walk) >> { >> const struct mm_walk_ops *ops = walk->ops; >> int err = 0; >> >> for (;;) { >> if (ops->install_pte && pte_none(ptep_get(pte))) { >> pte_t new_pte; >> >> err = ops->install_pte(addr, addr + PAGE_SIZE, &new_pte, >> walk); >> ---8<--- >> >> I agree that it's extremely confusing. Perhaps, at a minimum, we should come up >> with some kind of naming convention for this and update this and the other >> couple of places that pass pointers to stack-based pXX_t around? >> >> e.g. instead of calling it "ptep", call it "ptevalp" or something like that? > > Not sure that'd clarify, we already have a bit of an inconsistent mess with all > this :( > > Given it's a stack variable I'm not sure using a helper is in any way helpful > other than I suppose to account for people grepping around for incorrect page > table manipulation code? I've proposed an approach to clean all of this up. I'd appreciate your opinion if you get a few mins: https://lore.kernel.org/all/a063f6c5-2785-4a9f-8079-25edb3e54cef@arm.com/ > >> >> Thanks, >> Ryan >> >> >>> (*nr_pages)++; >>> >>> return 0; >> > > Cheers, Lorenzo