Re: [PATCH v1] mm/madvise: Always set ptes via arch helpers

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Fri, Mar 07, 2025 at 01:42:13PM +0000, Ryan Roberts wrote:
> On 07/03/2025 13:04, Lorenzo Stoakes wrote:
> > On Fri, Mar 07, 2025 at 12:33:06PM +0000, Ryan Roberts wrote:
> >> Instead of writing a pte directly into the table, use the set_pte_at()
> >> helper, which gives the arch visibility of the change.
> >>
> >> In this instance we are guaranteed that the pte was originally none and
> >> is being modified to a not-present pte, so there was unlikely to be a
> >> bug in practice (at least not on arm64). But it's bad practice to write
> >> the page table memory directly without arch involvement.
> >>
> >> Cc: <stable@vger.kernel.org>
> >> Fixes: 662df3e5c376 ("mm: madvise: implement lightweight guard page mechanism")
> >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> >> ---
> >>  mm/madvise.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/mm/madvise.c b/mm/madvise.c
> >> index 388dc289b5d1..6170f4acc14f 100644
> >> --- a/mm/madvise.c
> >> +++ b/mm/madvise.c
> >> @@ -1101,7 +1101,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_at(walk->mm, addr, ptep, make_pte_marker(PTE_MARKER_GUARD));
> >
> > I agree with you, but I think perhaps the arg name here is misleading :) If
> > you look at mm/pagewalk.c and specifically, in walk_pte_range_inner():
> >
> > 		if (ops->install_pte && pte_none(ptep_get(pte))) {
> > 			pte_t new_pte;
> >
> > 			err = ops->install_pte(addr, addr + PAGE_SIZE, &new_pte,
> > 					       walk);
> > 			if (err)
> > 				break;
> >
> > 			set_pte_at(walk->mm, addr, pte, new_pte);
> >
> > 			...
> > 		}
> >
> > So the ptep being assigned here is a stack value, new_pte, which we simply
> > assign to, and _then_ the page walker code handles the set_pte_at() for us.
> >
> > So we are indeed doing the right thing here, just in a different place :P
>
> Ahh my bad. In that case, please ignore the patch.
>
> But out of interest, why are you doing it like this? I find it a bit confusing
> as all the other ops (e.g. pte_entry()) work directly on the pgtable's pte
> without the intermediate.

In those cases it's read-only, the data's already there, you can just go ahead
and manipulate it (and would expect to be able to do so).

When setting things are a little different, I'd rather not open up things to a
user being able to do *whatever*, but rather limit to the smallest scope
possible for installing the PTE.

And also of course, it allows us to _mandate_ that set_pte_at() is used so we do
the right thing re: arches :)

I could have named the parameter better though, in guard_install_pte_entry()
would be better to have called it 'new_pte' or something.

>
> Thanks,
> Ryan
>
> >
> >>  	(*nr_pages)++;
> >>
> >>  	return 0;
> >> --
> >> 2.43.0
> >>
>

Thanks for looking at this by the way, obviously I appreciate your point in
chasing up cases like this as endeavoured to do the right thing here, albeit
abstracted away :)