On Thu, Mar 16, 2023 at 04:50PM +0800, Zhenhua Huang wrote: > Kfence only needs its pool to be mapped as page granularity, if it is > inited early. Previous judgement was a bit over protected. From [1], Mark > suggested to "just map the KFENCE region a page granularity". So I > decouple it from judgement and do page granularity mapping for kfence > pool only. Need to be noticed that late init of kfence pool still requires > page granularity mapping. > > Page granularity mapping in theory cost more(2M per 1GB) memory on arm64 > platform. Like what I've tested on QEMU(emulated 1GB RAM) with > gki_defconfig, also turning off rodata protection: > Before: > [root@liebao ]# cat /proc/meminfo > MemTotal: 999484 kB > After: > [root@liebao ]# cat /proc/meminfo > MemTotal: 1001480 kB > > To implement this, also relocate the kfence pool allocation before the > linear mapping setting up, arm64_kfence_alloc_pool is to allocate phys > addr, __kfence_pool is to be set after linear mapping set up. > > LINK: [1] https://lore.kernel.org/linux-arm-kernel/Y+IsdrvDNILA59UN@FVFF77S0Q05N/ > Suggested-by: Mark Rutland <mark.rutland@xxxxxxx> > Signed-off-by: Zhenhua Huang <quic_zhenhuah@xxxxxxxxxxx> > --- > arch/arm64/include/asm/kfence.h | 16 +++++++++++ > arch/arm64/mm/mmu.c | 59 +++++++++++++++++++++++++++++++++++++++++ > arch/arm64/mm/pageattr.c | 9 +++++-- > include/linux/kfence.h | 1 + > mm/kfence/core.c | 4 +++ > 5 files changed, 87 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/include/asm/kfence.h b/arch/arm64/include/asm/kfence.h > index aa855c6..8143c91 100644 > --- a/arch/arm64/include/asm/kfence.h > +++ b/arch/arm64/include/asm/kfence.h > @@ -10,6 +10,22 @@ > > #include <asm/set_memory.h> > > +extern phys_addr_t early_kfence_pool; This should not be accessible if !CONFIG_KFENCE. > +#ifdef CONFIG_KFENCE > + > +extern char *__kfence_pool; > +static inline void kfence_set_pool(phys_addr_t addr) > +{ > + __kfence_pool = phys_to_virt(addr); > +} kfence_set_pool() is redundant if it's for arm64 only, because we know where it's needed, and there you could just access __kfence_pool directly. So let's just remove this function. (Initially I thought you want to provide it generally, also for other architectures.) > +#else > + > +static inline void kfence_set_pool(phys_addr_t addr) { } > + > +#endif > + > static inline bool arch_kfence_init_pool(void) { return true; } [...] > +#endif > + > +phys_addr_t early_kfence_pool; This variable now exists in non-KFENCE builds, which is wrong. > static void __init map_mem(pgd_t *pgdp) > { > static const u64 direct_map_end = _PAGE_END(VA_BITS_MIN); > @@ -543,6 +587,10 @@ static void __init map_mem(pgd_t *pgdp) > */ > BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end)); > > + early_kfence_pool = arm64_kfence_alloc_pool(); > + if (early_kfence_pool) > + memblock_mark_nomap(early_kfence_pool, KFENCE_POOL_SIZE); > + > if (can_set_direct_map()) > flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS; > > @@ -608,6 +656,17 @@ static void __init map_mem(pgd_t *pgdp) > } > } > #endif > + > + /* Kfence pool needs page-level mapping */ > + if (early_kfence_pool) { > + __map_memblock(pgdp, early_kfence_pool, > + early_kfence_pool + KFENCE_POOL_SIZE, > + pgprot_tagged(PAGE_KERNEL), > + NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS); > + memblock_clear_nomap(early_kfence_pool, KFENCE_POOL_SIZE); > + /* kfence_pool really mapped now */ > + kfence_set_pool(early_kfence_pool); > + } This whole piece of code could also be wrapped in another function, which becomes a no-op if !CONFIG_KFENCE. Then you also don't need to provide the KFENCE_POOL_SIZE define for 0 if !CONFIG_KFENCE. [...] > + * > + * Kfence pool requires page granularity mapping also if we init it > + * late. > */ > return (rodata_enabled && rodata_full) || debug_pagealloc_enabled() || > - IS_ENABLED(CONFIG_KFENCE); > + (IS_ENABLED(CONFIG_KFENCE) && !early_kfence_pool); Accessing a non-existent variable if !CONFIG_KFENCE works because the compiler optimizes out the access, but is generally bad style. I think the only issue that I have is that the separation between KFENCE and non-KFENCE builds is not great. At the end of the email are is a diff against your patch which would be my suggested changes (while at it, I fixed up a bunch of other issues). Untested, so if you decide to adopt these changes, please test. Thanks, -- Marco ------ >8 ------ diff --git a/arch/arm64/include/asm/kfence.h b/arch/arm64/include/asm/kfence.h index 8143c91854e1..a81937fae9f6 100644 --- a/arch/arm64/include/asm/kfence.h +++ b/arch/arm64/include/asm/kfence.h @@ -10,22 +10,6 @@ #include <asm/set_memory.h> -extern phys_addr_t early_kfence_pool; - -#ifdef CONFIG_KFENCE - -extern char *__kfence_pool; -static inline void kfence_set_pool(phys_addr_t addr) -{ - __kfence_pool = phys_to_virt(addr); -} - -#else - -static inline void kfence_set_pool(phys_addr_t addr) { } - -#endif - static inline bool arch_kfence_init_pool(void) { return true; } static inline bool kfence_protect_page(unsigned long addr, bool protect) @@ -35,4 +19,14 @@ static inline bool kfence_protect_page(unsigned long addr, bool protect) return true; } +#ifdef CONFIG_KFENCE +extern bool kfence_early_init; +static inline bool arm64_kfence_can_set_direct_map(void) +{ + return !kfence_early_init; +} +#else /* CONFIG_KFENCE */ +static inline bool arm64_kfence_can_set_direct_map(void) { return false; } +#endif /* CONFIG_KFENCE */ + #endif /* __ASM_KFENCE_H */ diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index 61944c7091f0..683958616ac1 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -528,17 +528,14 @@ static int __init enable_crash_mem_map(char *arg) early_param("crashkernel", enable_crash_mem_map); #ifdef CONFIG_KFENCE +bool kfence_early_init = !!CONFIG_KFENCE_SAMPLE_INTERVAL; -static bool kfence_early_init __initdata = !!CONFIG_KFENCE_SAMPLE_INTERVAL; -/* - * early_param can be parsed before linear mapping - * set up - */ -static int __init parse_kfence_early_init(char *p) +/* early_param() will be parsed before map_mem() below. */ +static int __init parse_kfence_early_init(char *arg) { int val; - if (get_option(&p, &val)) + if (get_option(&arg, &val)) kfence_early_init = !!val; return 0; } @@ -552,22 +549,34 @@ static phys_addr_t arm64_kfence_alloc_pool(void) return 0; kfence_pool = memblock_phys_alloc(KFENCE_POOL_SIZE, PAGE_SIZE); - if (!kfence_pool) + if (!kfence_pool) { pr_err("failed to allocate kfence pool\n"); + kfence_early_init = false; + return 0; + } + + /* Temporarily mark as NOMAP. */ + memblock_mark_nomap(kfence_pool, KFENCE_POOL_SIZE); return kfence_pool; } -#else - -static phys_addr_t arm64_kfence_alloc_pool(void) +static void arm64_kfence_map_pool(phys_addr_t kfence_pool, pgd_t *pgdp) { - return 0; -} - -#endif + if (!kfence_pool) + return; -phys_addr_t early_kfence_pool; + /* KFENCE pool needs page-level mapping. */ + __map_memblock(pgdp, kfence_pool, kfence_pool + KFENCE_POOL_SIZE, + pgprot_tagged(PAGE_KERNEL), + NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS); + memblock_clear_nomap(kfence_pool, KFENCE_POOL_SIZE); + __kfence_pool = phys_to_virt(kfence_pool); +} +#else /* CONFIG_KFENCE */ +static inline phys_addr_t arm64_kfence_alloc_pool(void) { return 0; } +static inline void arm64_kfence_map_pool(phys_addr_t kfence_pool, pgd_t *pgdp) { } +#endif /* CONFIG_KFENCE */ static void __init map_mem(pgd_t *pgdp) { @@ -575,6 +584,7 @@ static void __init map_mem(pgd_t *pgdp) phys_addr_t kernel_start = __pa_symbol(_stext); phys_addr_t kernel_end = __pa_symbol(__init_begin); phys_addr_t start, end; + phys_addr_t early_kfence_pool; int flags = NO_EXEC_MAPPINGS; u64 i; @@ -588,8 +598,6 @@ static void __init map_mem(pgd_t *pgdp) BUILD_BUG_ON(pgd_index(direct_map_end - 1) == pgd_index(direct_map_end)); early_kfence_pool = arm64_kfence_alloc_pool(); - if (early_kfence_pool) - memblock_mark_nomap(early_kfence_pool, KFENCE_POOL_SIZE); if (can_set_direct_map()) flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS; @@ -656,17 +664,7 @@ static void __init map_mem(pgd_t *pgdp) } } #endif - - /* Kfence pool needs page-level mapping */ - if (early_kfence_pool) { - __map_memblock(pgdp, early_kfence_pool, - early_kfence_pool + KFENCE_POOL_SIZE, - pgprot_tagged(PAGE_KERNEL), - NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS); - memblock_clear_nomap(early_kfence_pool, KFENCE_POOL_SIZE); - /* kfence_pool really mapped now */ - kfence_set_pool(early_kfence_pool); - } + arm64_kfence_map_pool(early_kfence_pool, pgdp); } void mark_rodata_ro(void) diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c index 7ce5295cc6fb..aa8fd12cc96f 100644 --- a/arch/arm64/mm/pageattr.c +++ b/arch/arm64/mm/pageattr.c @@ -7,7 +7,6 @@ #include <linux/module.h> #include <linux/sched.h> #include <linux/vmalloc.h> -#include <linux/kfence.h> #include <asm/cacheflush.h> #include <asm/set_memory.h> @@ -28,11 +27,10 @@ bool can_set_direct_map(void) * mapped at page granularity, so that it is possible to * protect/unprotect single pages. * - * Kfence pool requires page granularity mapping also if we init it - * late. + * KFENCE pool requires page-granular mapping if initialized late. */ return (rodata_enabled && rodata_full) || debug_pagealloc_enabled() || - (IS_ENABLED(CONFIG_KFENCE) && !early_kfence_pool); + arm64_kfence_can_set_direct_map(); } static int change_page_range(pte_t *ptep, unsigned long addr, void *data) diff --git a/include/linux/kfence.h b/include/linux/kfence.h index 91cbcc98e293..726857a4b680 100644 --- a/include/linux/kfence.h +++ b/include/linux/kfence.h @@ -222,7 +222,6 @@ bool __kfence_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *sla #else /* CONFIG_KFENCE */ -#define KFENCE_POOL_SIZE 0 static inline bool is_kfence_address(const void *addr) { return false; } static inline void kfence_alloc_pool(void) { } static inline void kfence_init(void) { } diff --git a/mm/kfence/core.c b/mm/kfence/core.c index fab087d39633..e7f22af5e710 100644 --- a/mm/kfence/core.c +++ b/mm/kfence/core.c @@ -818,7 +818,7 @@ void __init kfence_alloc_pool(void) if (!kfence_sample_interval) return; - /* if the pool has already been initialized by arch, skip the below */ + /* If the pool has already been initialized by arch, skip the below. */ if (__kfence_pool) return;
