Re: [PATCH] mm/userfaultfd: fix missing PTE unmap for non-migration entries

[David Hildenbrand <david@redhat.com>]

On 07.08.25 21:51, Sasha Levin wrote:
> On Fri, Aug 01, 2025 at 10:29:17AM -0400, Sasha Levin wrote:
>> On Fri, Aug 01, 2025 at 04:06:14PM +0200, David Hildenbrand wrote:
>>> Sure, if it's prechecked by you no problem.
>>
>> Yup. Though I definitely learned a thing or two about Coccinelle patches
>> during this experiment.
> 
> Appologies if it isn't the case, but the two patches were attached to
> the previous mail and I suspect they might have been missed :)

Whoop's not used to reviewing attachments. I'll focus on the loongarch patch.

 From a547687db03ecfe13ddc74e452357df78f880255 Mon Sep 17 00:00:00 2001
From: Sasha Levin <sashal@kernel.org>
Date: Fri, 1 Aug 2025 09:17:04 -0400
Subject: [PATCH 2/2] LoongArch: fix kmap_local_page() LIFO ordering in
  copy_user_highpage()

The current implementation violates kmap_local_page()'s LIFO ordering
requirement by unmapping the pages in the same order they were mapped.

This was introduced by commit 477a0ebec101 ("LoongArch: Replace
kmap_atomic() with kmap_local_page() in copy_user_highpage()") when
converting from kmap_atomic() to kmap_local_page(). The original code
correctly unmapped in reverse order, but the conversion swapped the
mapping order while keeping the unmapping order unchanged, resulting
in a LIFO violation.

kmap_local_page() requires unmapping to be done in reverse order
(Last-In-First-Out). Currently we map vfrom and then vto, but unmap
vfrom and then vto, which is incorrect. This patch corrects it to
unmap vto first, then vfrom.

This issue was detected by the kmap_local_lifo.cocci semantic patch.

Fixes: 477a0ebec101 ("LoongArch: Replace kmap_atomic() with kmap_local_page() in copy_user_highpage()")
Co-developed-by: Claude claude-opus-4-20250514
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
  arch/loongarch/mm/init.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/loongarch/mm/init.c b/arch/loongarch/mm/init.c
index c3e4586a7975..01c43f455486 100644
--- a/arch/loongarch/mm/init.c
+++ b/arch/loongarch/mm/init.c
@@ -47,8 +47,8 @@ void copy_user_highpage(struct page *to, struct page *from,
  	vfrom = kmap_local_page(from);
  	vto = kmap_local_page(to);
  	copy_page(vto, vfrom);
-	kunmap_local(vfrom);
  	kunmap_local(vto);
+	kunmap_local(vfrom);
  	/* Make sure this page is cleared on other CPU's too before using it */
  	smp_wmb();
  }
-- 
2.39.5


So, loongarch neither supports

a) Highmem

nor

b) ARCH_SUPPORTS_KMAP_LOCAL_FORCE_MAP, disabling DEBUG_KMAP_LOCAL_FORCE_MAP

Consequently __kmap_local_page_prot() will not do anything:

	if (!IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) && !PageHighMem(page))
		return page_address(page);


So there isn't anything to fix here and the whole patch subject+description should be
rewritten to focus on this being purely a cleanup -- unless I am missing
something important.

Also, please reduce the description to the absolute minimum, nobody wants to read the
same thing 4 times using slightly different words.

"LIFO ordering", "LIFO ordering", "unmapped in reverse order ... LIFO violation" ...
"reverse order (Last-In-First-Out)"


More importantly: is the LIFO semantics clearly documented somewhere? I read
Documentation/mm/highmem.rst

   Nesting kmap_local_page() and kmap_atomic() mappings is allowed to a certain
   extent (up to KMAP_TYPE_NR) but their invocations have to be strictly ordered
   because the map implementation is stack based. See kmap_local_page() kdocs
   (included in the "Functions" section) for details on how to manage nested
   mappings.

and that kind-of spells that out (strictly order -> stack based). I think one could
have clarified that a bit further.

Also, I would expect this to be mentioned in the docs of the relevant kmap functions,
and the pte map / unmap functions.

Did I miss that part or could we extend the function docs to spell that out?

-- 
Cheers,

David / dhildenb