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 729FD21578F for ; Thu, 11 Dec 2025 09:44:03 +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=1765446245; cv=none; b=VyKrdBKS4GtTcfx0RkOei70pqLEUjDZatpd34wXYzgk9fh21LEXlK4sjBm7wJO082smijoGOs4ESpoLbQ33t1cd1XVd6aySwEVzOCDPrLPovpKuwxzBefsaNV/cg2QW7JMiRZf2rr3U6WK2HPdYfLYx7611UKTg7//DBTAS73PQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765446245; c=relaxed/simple; bh=T9kMMP0mWjFGs8bAHKFSSZxE3D9yqFVquNoMiwpB8KQ=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=X76zfMCslktmLnRVsZR/iY9BDt8KGcVog0C9c/Pjc/hxRMEJtZrdIWOjLOHB1kbVWzNkgE/uFtFIG33ZMchbgx9H1eiOcg9N7UKzLdNdWSWKYYEYFNbhQTc3R1a5GM1g23M8G+10wm3n03NJ9T4thOCGdX9wNJhbh/HEiNZAG70= 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 7E1CE175D; Thu, 11 Dec 2025 01:43:55 -0800 (PST) Received: from [10.57.90.205] (unknown [10.57.90.205]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 91E593F740; Thu, 11 Dec 2025 01:44:00 -0800 (PST) Message-ID: <1b7e682e-86a4-4573-b423-65f9755f71ea@arm.com> Date: Thu, 11 Dec 2025 09:43:58 +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: Samuel Holland , Andrew Morton , "Liam R . Howlett" , Lorenzo Stoakes , David Hildenbrand , Vlastimil Babka , Jann Horn Cc: 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> From: Ryan Roberts In-Reply-To: <20251211081117.1126521-3-samuel.holland@sifive.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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(). 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? Thanks, Ryan > (*nr_pages)++; > > return 0;