[PATCH v3] mm,kfence: decouple kfence from page granularity mapping judgement

[PATCH v3] mm,kfence: decouple kfence from page granularity mapping judgement

[Zhenhua Huang]

Kfence only needs its pool to be mapped as page granularity, previous
judgement was a bit over protected. Decouple it from judgement and do
page granularity mapping for kfence pool only [1].

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/1675750519-1064-1-git-send-email-quic_zhenhuah@quicinc.com/T/
Suggested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
---
 arch/arm64/mm/mmu.c      | 44 ++++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/pageattr.c |  5 ++---
 include/linux/kfence.h   |  8 ++++++++
 mm/kfence/core.c         |  9 +++++++++
 4 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 6f9d889..9f06a29e 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -24,6 +24,7 @@
 #include <linux/mm.h>
 #include <linux/vmalloc.h>
 #include <linux/set_memory.h>
+#include <linux/kfence.h>
 
 #include <asm/barrier.h>
 #include <asm/cputype.h>
@@ -525,6 +526,33 @@ static int __init enable_crash_mem_map(char *arg)
 }
 early_param("crashkernel", enable_crash_mem_map);
 
+#ifdef CONFIG_KFENCE
+
+static phys_addr_t arm64_kfence_alloc_pool(void)
+{
+	phys_addr_t kfence_pool = 0;
+
+	if (!kfence_sample_interval)
+		return (phys_addr_t)NULL;
+
+	kfence_pool = memblock_phys_alloc(KFENCE_POOL_SIZE, PAGE_SIZE);
+	if (!kfence_pool) {
+		pr_err("failed to allocate kfence pool\n");
+		return (phys_addr_t)NULL;
+	}
+
+	return kfence_pool;
+}
+
+#else
+
+static phys_addr_t arm64_kfence_alloc_pool(void)
+{
+	return (phys_addr_t)NULL;
+}
+
+#endif
+
 static void __init map_mem(pgd_t *pgdp)
 {
 	static const u64 direct_map_end = _PAGE_END(VA_BITS_MIN);
@@ -532,6 +560,7 @@ static void __init map_mem(pgd_t *pgdp)
 	phys_addr_t kernel_end = __pa_symbol(__init_begin);
 	phys_addr_t start, end;
 	int flags = NO_EXEC_MAPPINGS;
+	phys_addr_t kfence_pool = 0;
 	u64 i;
 
 	/*
@@ -564,6 +593,10 @@ static void __init map_mem(pgd_t *pgdp)
 	}
 #endif
 
+	kfence_pool = arm64_kfence_alloc_pool();
+	if (kfence_pool)
+		memblock_mark_nomap(kfence_pool, KFENCE_POOL_SIZE);
+
 	/* map all the memory banks */
 	for_each_mem_range(i, &start, &end) {
 		if (start >= end)
@@ -608,6 +641,17 @@ static void __init map_mem(pgd_t *pgdp)
 		}
 	}
 #endif
+
+	/* Kfence pool needs page-level mapping */
+	if (kfence_pool) {
+		__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 really mapped now */
+		kfence_set_pool(kfence_pool);
+	}
 }
 
 void mark_rodata_ro(void)
diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 79dd201..61156d0 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c
@@ -22,12 +22,11 @@ bool rodata_full __ro_after_init = IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED
 bool can_set_direct_map(void)
 {
 	/*
-	 * rodata_full, DEBUG_PAGEALLOC and KFENCE require linear map to be
+	 * rodata_full and DEBUG_PAGEALLOC require linear map to be
 	 * mapped at page granularity, so that it is possible to
 	 * protect/unprotect single pages.
 	 */
-	return (rodata_enabled && rodata_full) || debug_pagealloc_enabled() ||
-		IS_ENABLED(CONFIG_KFENCE);
+	return (rodata_enabled && rodata_full) || debug_pagealloc_enabled();
 }
 
 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 726857a..570d4e3 100644
--- a/include/linux/kfence.h
+++ b/include/linux/kfence.h
@@ -64,6 +64,12 @@ static __always_inline bool is_kfence_address(const void *addr)
 void __init kfence_alloc_pool(void);
 
 /**
+ * kfence_set_pool() - allows an arch to set the
+ * KFENCE pool during early init
+ */
+void __init kfence_set_pool(phys_addr_t addr);
+
+/**
  * kfence_init() - perform KFENCE initialization at boot time
  *
  * Requires that kfence_alloc_pool() was called before. This sets up the
@@ -222,8 +228,10 @@ 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_set_pool(phys_addr_t addr) { }
 static inline void kfence_init(void) { }
 static inline void kfence_shutdown_cache(struct kmem_cache *s) { }
 static inline void *kfence_alloc(struct kmem_cache *s, size_t size, gfp_t flags) { return NULL; }
diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 5349c37..0765395 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -814,12 +814,21 @@ void __init kfence_alloc_pool(void)
 	if (!kfence_sample_interval)
 		return;
 
+	/* if the pool has already been initialized by arch, skip the below */
+	if (__kfence_pool)
+		return;
+
 	__kfence_pool = memblock_alloc(KFENCE_POOL_SIZE, PAGE_SIZE);
 
 	if (!__kfence_pool)
 		pr_err("failed to allocate pool\n");
 }
 
+void __init kfence_set_pool(phys_addr_t addr)
+{
+	__kfence_pool = phys_to_virt(addr);
+}
+
 static void kfence_init_enable(void)
 {
 	if (!IS_ENABLED(CONFIG_KFENCE_STATIC_KEYS))
-- 
2.7.4

Re: [PATCH v3] mm,kfence: decouple kfence from page granularity mapping judgement

[Kefeng Wang]

Hi Zhenhua,

On 2023/3/10 10:02, Zhenhua Huang wrote:
> Kfence only needs its pool to be mapped as page granularity, previous
> judgement was a bit over protected. Decouple it from judgement and do
> page granularity mapping for kfence pool only [1].
> 
> 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.
> 
We do the same way in our 5.10 kernel, a minor comment below,

> LINK: [1] https://lore.kernel.org/linux-arm-kernel/1675750519-1064-1-git-send-email-quic_zhenhuah@quicinc.com/T/
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
> ---
>   arch/arm64/mm/mmu.c      | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   arch/arm64/mm/pageattr.c |  5 ++---
>   include/linux/kfence.h   |  8 ++++++++
>   mm/kfence/core.c         |  9 +++++++++
>   4 files changed, 63 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 6f9d889..9f06a29e 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -24,6 +24,7 @@
>   #include <linux/mm.h>
>   #include <linux/vmalloc.h>
>   #include <linux/set_memory.h>
> +#include <linux/kfence.h>
>   
>   #include <asm/barrier.h>
>   #include <asm/cputype.h>
> @@ -525,6 +526,33 @@ static int __init enable_crash_mem_map(char *arg)
>   }
>   early_param("crashkernel", enable_crash_mem_map);
>   
> +#ifdef CONFIG_KFENCE
> +
> +static phys_addr_t arm64_kfence_alloc_pool(void)
> +{
> +	phys_addr_t kfence_pool = 0;

The kfence_pool is no need to be initialized.

> +
> +	if (!kfence_sample_interval)
> +		return (phys_addr_t)NULL;

And one more missing case, kfence support late int, see commit
b33f778bba5e ("kfence: alloc kfence_pool after system startup"),
this changes will break this feature, we add a new cmdline to alloc
kfence_pool regardless of kfence_sample_interval value, maybe there some
other way to deal with this issue.

> +
> +	kfence_pool = memblock_phys_alloc(KFENCE_POOL_SIZE, PAGE_SIZE);
> +	if (!kfence_pool) {
> +		pr_err("failed to allocate kfence pool\n");
> +		return (phys_addr_t)NULL;

no need this return;

> +	}

> +
> +	return kfence_pool;
> +}
> +
> +#else
> +
> +static phys_addr_t arm64_kfence_alloc_pool(void)
> +{
> +	return (phys_addr_t)NULL;
> +}
> +
> +#endif
> +

I like all of '(phys_addr_t)NULL' to 0

>   static void __init map_mem(pgd_t *pgdp)
>   {
>   	static const u64 direct_map_end = _PAGE_END(VA_BITS_MIN);
> @@ -532,6 +560,7 @@ static void __init map_mem(pgd_t *pgdp)
>   	phys_addr_t kernel_end = __pa_symbol(__init_begin);
>   	phys_addr_t start, end;
>   	int flags = NO_EXEC_MAPPINGS;
> +	phys_addr_t kfence_pool = 0;

it's no need to be initialized too.

>   	u64 i;
>   
>   	/*
> @@ -564,6 +593,10 @@ static void __init map_mem(pgd_t *pgdp)
>   	}
>   #endif
>   
> +	kfence_pool = arm64_kfence_alloc_pool();
> +	if (kfence_pool)
> +		memblock_mark_nomap(kfence_pool, KFENCE_POOL_SIZE);
> +
>   	/* map all the memory banks */
>   	for_each_mem_range(i, &start, &end) {
>   		if (start >= end)
> @@ -608,6 +641,17 @@ static void __init map_mem(pgd_t *pgdp)
>   		}
>   	}
>   #endif
> +
> +	/* Kfence pool needs page-level mapping */
> +	if (kfence_pool) {
> +		__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 really mapped now */
> +		kfence_set_pool(kfence_pool);
> +	}
>   }
>   

Re: [PATCH v3] mm,kfence: decouple kfence from page granularity mapping judgement

[Zhenhua Huang]

Appreciate Kefeng for your review!

On 2023/3/10 10:56, Kefeng Wang wrote:
> 
> Hi Zhenhua,
> 
> On 2023/3/10 10:02, Zhenhua Huang wrote:
>> Kfence only needs its pool to be mapped as page granularity, previous
>> judgement was a bit over protected. Decouple it from judgement and do
>> page granularity mapping for kfence pool only [1].
>>
>> 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.
>>
> We do the same way in our 5.10 kernel, a minor comment below,

Yeah.. low memory device can benefit from this.

> 
>> LINK: [1] 
>> https://lore.kernel.org/linux-arm-kernel/1675750519-1064-1-git-send-email-quic_zhenhuah@quicinc.com/T/
>> Suggested-by: Mark Rutland <mark.rutland@arm.com>
>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
>> ---
>>   arch/arm64/mm/mmu.c      | 44 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   arch/arm64/mm/pageattr.c |  5 ++---
>>   include/linux/kfence.h   |  8 ++++++++
>>   mm/kfence/core.c         |  9 +++++++++
>>   4 files changed, 63 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 6f9d889..9f06a29e 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/mm.h>
>>   #include <linux/vmalloc.h>
>>   #include <linux/set_memory.h>
>> +#include <linux/kfence.h>
>>   #include <asm/barrier.h>
>>   #include <asm/cputype.h>
>> @@ -525,6 +526,33 @@ static int __init enable_crash_mem_map(char *arg)
>>   }
>>   early_param("crashkernel", enable_crash_mem_map);
>> +#ifdef CONFIG_KFENCE
>> +
>> +static phys_addr_t arm64_kfence_alloc_pool(void)
>> +{
>> +    phys_addr_t kfence_pool = 0;
> 
> The kfence_pool is no need to be initialized.

Done

> 
>> +
>> +    if (!kfence_sample_interval)
>> +        return (phys_addr_t)NULL;
> 
> And one more missing case, kfence support late int, see commit
> b33f778bba5e ("kfence: alloc kfence_pool after system startup"),
> this changes will break this feature, we add a new cmdline to alloc
> kfence_pool regardless of kfence_sample_interval value, maybe there some
> other way to deal with this issue.

Yeah, Thanks for reminder. It seems we need only to avoid the case which 
allocating pool later. kfence_pool also seems only to be allocated once, 
and once allocated, will not be freed any more. So how about we raise 
another change, like you mentioned bootargs indicating using feature of 
b33f778bba5e ("kfence: alloc kfence_pool after system startup").
1. in arm64_kfence_alloc_pool():
    if (!kfence_sample_interval && !bootargs)
              return 0;
    else
              allocate pool
2. also do the check in late allocation,like
    if (do_allocation_late && !bootargs)
              BUG();

> 
>> +
>> +    kfence_pool = memblock_phys_alloc(KFENCE_POOL_SIZE, PAGE_SIZE);
>> +    if (!kfence_pool) {
>> +        pr_err("failed to allocate kfence pool\n");
>> +        return (phys_addr_t)NULL;
> 
> no need this return;

Done

> 
>> +    }
> 
>> +
>> +    return kfence_pool;
>> +}
>> +
>> +#else
>> +
>> +static phys_addr_t arm64_kfence_alloc_pool(void)
>> +{
>> +    return (phys_addr_t)NULL;
>> +}
>> +
>> +#endif
>> +
> 
> I like all of '(phys_addr_t)NULL' to 0

I've tried, yeah, seems no warning. Updated.

> 
>>   static void __init map_mem(pgd_t *pgdp)
>>   {
>>       static const u64 direct_map_end = _PAGE_END(VA_BITS_MIN);
>> @@ -532,6 +560,7 @@ static void __init map_mem(pgd_t *pgdp)
>>       phys_addr_t kernel_end = __pa_symbol(__init_begin);
>>       phys_addr_t start, end;
>>       int flags = NO_EXEC_MAPPINGS;
>> +    phys_addr_t kfence_pool = 0;
> 
> it's no need to be initialized too.

Done

> 
>>       u64 i;
>>       /*
>> @@ -564,6 +593,10 @@ static void __init map_mem(pgd_t *pgdp)
>>       }
>>   #endif
>> +    kfence_pool = arm64_kfence_alloc_pool();
>> +    if (kfence_pool)
>> +        memblock_mark_nomap(kfence_pool, KFENCE_POOL_SIZE);
>> +
>>       /* map all the memory banks */
>>       for_each_mem_range(i, &start, &end) {
>>           if (start >= end)
>> @@ -608,6 +641,17 @@ static void __init map_mem(pgd_t *pgdp)
>>           }
>>       }
>>   #endif
>> +
>> +    /* Kfence pool needs page-level mapping */
>> +    if (kfence_pool) {
>> +        __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 really mapped now */
>> +        kfence_set_pool(kfence_pool);
>> +    }
>>   }
> 

Addressed above comments, I've raised V4 patchset, please help review:)