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

From: Zi Yan
Date: Sat Dec 07 2024 - 11:22:51 EST

Next message: Zi Yan: "Re: [PATCH v2] mm: use clear_user_(high)page() for arch with special user folio handling"
Previous message: Rob Clark: "[RFC 24/24] drm/msm: Bump UAPI version"
In reply to: Mathieu Desnoyers: "Re: [PATCH v2] mm: use clear_user_(high)page() for arch with special user folio handling"
Next in thread: Zi Yan: "Re: [PATCH v2] mm: use clear_user_(high)page() for arch with special user folio handling"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
>> Closes: https://lore.kernel.org/linux-mm/CAMuHMdV1hRp_NtR5YnJo=HsfgKQeH91J537Gh4gKk3PFZhSkbA@xxxxxxxxxxxxxx/
>> Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>> Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
>> ---
>> 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

Next message: Zi Yan: "Re: [PATCH v2] mm: use clear_user_(high)page() for arch with special user folio handling"
Previous message: Rob Clark: "[RFC 24/24] drm/msm: Bump UAPI version"
In reply to: Mathieu Desnoyers: "Re: [PATCH v2] mm: use clear_user_(high)page() for arch with special user folio handling"
Next in thread: Zi Yan: "Re: [PATCH v2] mm: use clear_user_(high)page() for arch with special user folio handling"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]