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

[Sasha Levin <sashal@kernel.org>]

On Fri, Aug 08, 2025 at 10:02:08AM +0200, David Hildenbrand wrote:
>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.

Thank you for the review!

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

How about something like:

     LoongArch: cleanup kmap_local_page() usage in copy_user_highpage()
     
     Unmap kmap_local_page() mappings in reverse order to follow the
     function's LIFO specification. While LoongArch doesn't support
     highmem and these operations are no-ops, code should still adhere
     to the API requirements.
     
     Detected by kmap_local_lifo.cocci.
     
     Fixes: 477a0ebec101 ("LoongArch: Replace kmap_atomic() with kmap_local_page() in copy_user_highpage()")
     Signed-off-by: Sasha Levin <sashal@kernel.org>

>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?

The docs for kmap_local_page() seem to cover it better, and give the
concrete example we're trying to fix here:

  * Requires careful handling when nesting multiple mappings because the map
  * management is stack based. The unmap has to be in the reverse order of
  * the map operation:
  *
  * addr1 = kmap_local_page(page1);
  * addr2 = kmap_local_page(page2);
  * ...
  * kunmap_local(addr2);
  * kunmap_local(addr1);
  *
  * Unmapping addr1 before addr2 is invalid and causes malfunction.

-- 
Thanks,
Sasha