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

[Sasha Levin]

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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>

> 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