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 :)