Re: [PATCH v2] mm: use clear_user_(high)page() for arch with special user folio handling

[Zi Yan <ziy@nvidia.com>]

On 7 Dec 2024, at 10:31, Mathieu Desnoyers wrote:

> On 2024-12-06 12:42, Zi Yan wrote:
>> For architectures setting cpu_dcache_is_aliasing() to true, which require
>> flushing cache, and arc, which changes folio->flags after clearing a user
>> folio, __GFP_ZERO using only clear_page() is not enough to zero user
>> folios and clear_user_(high)page() must be used. Otherwise, user data
>> will be corrupted.
>>
>> Fix it by always clearing user folios with clear_user_(high)page() when
>> cpu_dcache_is_aliasing() is true or architecture is arc. Rename
>> alloc_zeroed() to alloc_need_zeroing() and invert the logic to clarify its
>> intend.
>>
>> Fixes: 5708d96da20b ("mm: avoid zeroing user movable page twice with init_on_alloc=1")
>> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Closes: https://lore.kernel.org/linux-mm/CAMuHMdV1hRp_NtR5YnJo=HsfgKQeH91J537Gh4gKk3PFZhSkbA@mail.gmail.com/
>> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> ---
>>   include/linux/highmem.h |  8 +++++++-
>>   include/linux/mm.h      | 17 +++++++++++++++++
>>   mm/huge_memory.c        |  9 +++++----
>>   mm/internal.h           |  6 ------
>>   mm/memory.c             | 10 +++++-----
>>   5 files changed, 34 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
>> index 6e452bd8e7e3..d9beb8371daa 100644
>> --- a/include/linux/highmem.h
>> +++ b/include/linux/highmem.h
>> @@ -224,7 +224,13 @@ static inline
>>   struct folio *vma_alloc_zeroed_movable_folio(struct vm_area_struct *vma,
>>   				   unsigned long vaddr)
>>   {
>> -	return vma_alloc_folio(GFP_HIGHUSER_MOVABLE | __GFP_ZERO, 0, vma, vaddr);
>> +	struct folio *folio;
>> +
>> +	folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, vma, vaddr);
>> +	if (folio && alloc_need_zeroing())
>> +		clear_user_highpage(&folio->page, vaddr);
>> +
>> +	return folio;
>>   }
>>   #endif
>>  diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index c39c4945946c..ca8df5871213 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -31,6 +31,7 @@
>>   #include <linux/kasan.h>
>>   #include <linux/memremap.h>
>>   #include <linux/slab.h>
>> +#include <linux/cacheinfo.h>
>>    struct mempolicy;
>>   struct anon_vma;
>> @@ -4175,6 +4176,22 @@ static inline int do_mseal(unsigned long start, size_t len_in, unsigned long fla
>>   }
>>   #endif
>>  +/*
>> + * alloc_need_zeroing checks if a user folio from page allocator needs to be
>> + * zeroed or not.
>> + */
>> +static inline bool alloc_need_zeroing(void)
>> +{
>> +	/*
>> +	 * for user folios, arch with cache aliasing requires cache flush and
>> +	 * arc changes folio->flags, so always return false to make caller use
>> +	 * clear_user_page()/clear_user_highpage()
>> +	 */
>> +	return (cpu_dcache_is_aliasing() || IS_ENABLED(CONFIG_ARC)) ||
>
> Nack.
>
> Can we please not go back to re-introducing arch-specific
> conditionals in generic mm code after the cleanup I did when
> introducing cpu_dcache_is_aliasing() in commit 8690bbcf3b70 ?

OK

>
> Based on commit eacd0e950dc2, AFAIU what you appear to need here
> is to introduce a:
>
> cpu_icache_is_aliasing() -> note the "i" for instruction cache
>
> It would typically be directly set to
>
> #define cpu_icache_is_aliasing() cpu_dcache_is_aliasing()
>
> except on architecture like ARC when the icache vs dcache
> is aliasing, but not dcache vs dcache.
>
> So for ARC it would be defined as:
>
> #define cpu_dcache_is_aliasing() false
> #define cpu_icache_is_aliasing() true
>
> And the Kconfig ARCH_HAS_CPU_CACHE_ALIASING=y would be set for ARC
> again.
>

Sounds good to me.

> I'm not entirely sure if we want to go for the wording "is_aliasing"
> or "is_incoherent" when talking about icache vs dcache, so I'm open
> to ideas here.
>

Let me know if the code below looks good to you. I will use
(cpu_icache_is_aliasing() || cpu_dcache_is_aliasing())
instead of
(cpu_dcache_is_aliasing() || IS_ENABLED(CONFIG_ARC))
in my next version.

diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
index 5b2488142041..e96935373796 100644
--- a/arch/arc/Kconfig
+++ b/arch/arc/Kconfig
@@ -6,6 +6,7 @@
 config ARC
 	def_bool y
 	select ARC_TIMERS
+	select ARCH_HAS_CPU_CACHE_ALIASING
 	select ARCH_HAS_CACHE_LINE_SIZE
 	select ARCH_HAS_DEBUG_VM_PGTABLE
 	select ARCH_HAS_DMA_PREP_COHERENT
diff --git a/arch/arc/include/asm/cachetype.h b/arch/arc/include/asm/cachetype.h
new file mode 100644
index 000000000000..acd3b6cb4bf5
--- /dev/null
+++ b/arch/arc/include/asm/cachetype.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_ARC_CACHETYPE_H
+#define __ASM_ARC_CACHETYPE_H
+
+#define cpu_dcache_is_aliasing()	false
+#define cpu_icache_is_aliasing()	true
+
+#endif
diff --git a/arch/arm/include/asm/cachetype.h b/arch/arm/include/asm/cachetype.h
index b9dbe1d4c8fe..80725010c38f 100644
--- a/arch/arm/include/asm/cachetype.h
+++ b/arch/arm/include/asm/cachetype.h
@@ -21,6 +21,7 @@ extern unsigned int cacheid;
 #define icache_is_pipt()		cacheid_is(CACHEID_PIPT)

 #define cpu_dcache_is_aliasing()	(cache_is_vivt() || cache_is_vipt_aliasing())
+#define cpu_icache_is_aliasing() cpu_dcache_is_aliasing()

 /*
  * __LINUX_ARM_ARCH__ is the minimum supported CPU architecture
diff --git a/arch/csky/include/asm/cachetype.h b/arch/csky/include/asm/cachetype.h
index 98cbe3af662f..838c7e708d19 100644
--- a/arch/csky/include/asm/cachetype.h
+++ b/arch/csky/include/asm/cachetype.h
@@ -5,5 +5,6 @@
 #include <linux/types.h>

 #define cpu_dcache_is_aliasing()	true
+#define cpu_icache_is_aliasing() cpu_dcache_is_aliasing()

 #endif
diff --git a/arch/m68k/include/asm/cachetype.h b/arch/m68k/include/asm/cachetype.h
index 7fad5d9ab8fe..4993846354f4 100644
--- a/arch/m68k/include/asm/cachetype.h
+++ b/arch/m68k/include/asm/cachetype.h
@@ -5,5 +5,6 @@
 #include <linux/types.h>

 #define cpu_dcache_is_aliasing()	true
+#define cpu_icache_is_aliasing() cpu_dcache_is_aliasing()

 #endif
diff --git a/arch/mips/include/asm/cachetype.h b/arch/mips/include/asm/cachetype.h
index 9f4ba2fe1155..aac25ee27cd1 100644
--- a/arch/mips/include/asm/cachetype.h
+++ b/arch/mips/include/asm/cachetype.h
@@ -5,5 +5,6 @@
 #include <asm/cpu-features.h>

 #define cpu_dcache_is_aliasing()	cpu_has_dc_aliases
+#define cpu_icache_is_aliasing() cpu_dcache_is_aliasing()

 #endif
diff --git a/arch/nios2/include/asm/cachetype.h b/arch/nios2/include/asm/cachetype.h
index eb9c416b8a1c..4b492fbb4a39 100644
--- a/arch/nios2/include/asm/cachetype.h
+++ b/arch/nios2/include/asm/cachetype.h
@@ -6,5 +6,6 @@
 #include <asm/cache.h>

 #define cpu_dcache_is_aliasing()	(NIOS2_DCACHE_SIZE > PAGE_SIZE)
+#define cpu_icache_is_aliasing() cpu_dcache_is_aliasing()

 #endif
diff --git a/arch/parisc/include/asm/cachetype.h b/arch/parisc/include/asm/cachetype.h
index e0868a1d3c47..53180ff9d09a 100644
--- a/arch/parisc/include/asm/cachetype.h
+++ b/arch/parisc/include/asm/cachetype.h
@@ -5,5 +5,6 @@
 #include <linux/types.h>

 #define cpu_dcache_is_aliasing()	true
+#define cpu_icache_is_aliasing() cpu_dcache_is_aliasing()

 #endif
diff --git a/arch/sh/include/asm/cachetype.h b/arch/sh/include/asm/cachetype.h
index a5fffe536068..f711631b9451 100644
--- a/arch/sh/include/asm/cachetype.h
+++ b/arch/sh/include/asm/cachetype.h
@@ -5,5 +5,6 @@
 #include <linux/types.h>

 #define cpu_dcache_is_aliasing()	true
+#define cpu_icache_is_aliasing() cpu_dcache_is_aliasing()

 #endif
diff --git a/arch/xtensa/include/asm/cachetype.h b/arch/xtensa/include/asm/cachetype.h
index 51bd49e2a1c5..89560b325006 100644
--- a/arch/xtensa/include/asm/cachetype.h
+++ b/arch/xtensa/include/asm/cachetype.h
@@ -6,5 +6,6 @@
 #include <asm/page.h>

 #define cpu_dcache_is_aliasing()	(DCACHE_WAY_SIZE > PAGE_SIZE)
+#define cpu_icache_is_aliasing() cpu_dcache_is_aliasing()

 #endif
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 108060612bb8..a2dd3d00e080 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -155,6 +155,7 @@ static inline int get_cpu_cacheinfo_id(int cpu, int level)

 #ifndef CONFIG_ARCH_HAS_CPU_CACHE_ALIASING
 #define cpu_dcache_is_aliasing()	false
+#define cpu_icache_is_aliasing() cpu_dcache_is_aliasing()
 #else
 #include <asm/cachetype.h>
 #endif



Best Regards,
Yan, Zi