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.