Re: [PATCH v2] radix/kfence: map __kfence_pool at page granularity

[Michael Ellerman <mpe@ellerman.id.au>]

Hi Hari,

Just a few little things ...

Hari Bathini <hbathini@linux.ibm.com> writes:
> When KFENCE is enabled, total system memory is mapped at page level
> granularity. But in radix MMU mode, ~3GB additional memory is needed
> to map 100GB of system memory at page level granularity when compared
> to using 2MB direct mapping. This is not desired considering KFENCE is
> designed to be enabled in production kernels [1].
> Also, mapping memory
> allocated for KFENCE pool at page granularity seems sufficient enough
> to enable KFENCE support.

This should be firmer, eg:
  Mapping only the memory allocated for KFENCE pool at page granularity is sufficient to enable KFENCE support.

> So, allocate __kfence_pool during bootup and
> map it at page granularity instead of mapping all system memory at
> page granularity.
>
> Without patch:
>     # cat /proc/meminfo
>     MemTotal:       101201920 kB
>
> With patch:
>     # cat /proc/meminfo
>     MemTotal:       104483904 kB
>
> Note that enabling KFENCE at runtime is disabled for radix MMU for now,
> as it depends on the ability to split page table mappings and such APIs
> are not currently implemented for radix MMU.
>
> All kfence_test.c testcases passed with this patch.
>
> [1] https://lore.kernel.org/all/20201103175841.3495947-2-elver@google.com/
>
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> ---
>
> Changes in v2:
> * Dropped the patch that adds support to enable KFENCE after startup.
> * Added changes to avoid KFENCE enablement after system startup.
> * Also, added a TODO explaining why KFENCE enablement after startup
>   is not supported for now.
> * Functions to alloc/map __kfence_pool as suggested by Ritesh.
> * Moved changes that apply to ppc32 as well to common file as suggested
>   by Christophe.
>
>
>  arch/powerpc/include/asm/kfence.h        | 12 +++-
>  arch/powerpc/mm/book3s64/radix_pgtable.c | 74 ++++++++++++++++++++++--
>  arch/powerpc/mm/init-common.c            | 14 +++++
>  3 files changed, 95 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kfence.h b/arch/powerpc/include/asm/kfence.h
> index 424ceef82ae6..78590288ee80 100644
> --- a/arch/powerpc/include/asm/kfence.h
> +++ b/arch/powerpc/include/asm/kfence.h
> @@ -15,10 +15,20 @@
>  #define ARCH_FUNC_PREFIX "."
>  #endif
>  
> +#ifdef CONFIG_KFENCE
> +extern bool kfence_early_init;
 
This is only read in radix_pgtable.c, so it needed be global.

> +extern bool kfence_disabled;
> +
> +static inline void disable_kfence(void)
> +{
> +	kfence_disabled = true;
> +}
> +
>  static inline bool arch_kfence_init_pool(void)
>  {
> -	return true;
> +	return !kfence_disabled;
>  }
> +#endif
>  
>  #ifdef CONFIG_PPC64
>  static inline bool kfence_protect_page(unsigned long addr, bool protect)
> diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
> index 15e88f1439ec..a74912e0fd99 100644
> --- a/arch/powerpc/mm/book3s64/radix_pgtable.c
> +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
> @@ -301,7 +304,10 @@ static int __meminit create_physical_mapping(unsigned long start,
>  	int psize;
>  	unsigned long max_mapping_size = memory_block_size;
>  
> -	if (debug_pagealloc_enabled_or_kfence())
> +	if (mapping_sz_limit < max_mapping_size)
> +		max_mapping_size = mapping_sz_limit;
> +
> +	if (debug_pagealloc_enabled())
>  		max_mapping_size = PAGE_SIZE;
>  
>  	start = ALIGN(start, PAGE_SIZE);
> @@ -356,8 +362,64 @@ static int __meminit create_physical_mapping(unsigned long start,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_KFENCE
> +static inline phys_addr_t radix_alloc_kfence_pool_early(void)
 
This is internal to radix_pgtable.c, so I think we can drop the radix
prefix. There's also no alloc_kfence_pool(), so early is not really
required IMHO. So it could just be alloc_kfence_pool(). Similar for map.

...
> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
> index d3a7726ecf51..f881ab5107aa 100644
> --- a/arch/powerpc/mm/init-common.c
> +++ b/arch/powerpc/mm/init-common.c
> @@ -31,6 +31,20 @@ EXPORT_SYMBOL_GPL(kernstart_virt_addr);
>  
>  bool disable_kuep = !IS_ENABLED(CONFIG_PPC_KUEP);
>  bool disable_kuap = !IS_ENABLED(CONFIG_PPC_KUAP);
> +#ifdef CONFIG_KFENCE
> +bool __ro_after_init kfence_early_init = !!CONFIG_KFENCE_SAMPLE_INTERVAL;
> +bool __ro_after_init kfence_disabled;
> +
> +static int __init parse_kfence_early_init(char *arg)
> +{
> +	int val;
> +
> +	if (get_option(&arg, &val))
> +		kfence_early_init = !!val;
> +	return 0;
> +}
> +early_param("kfence.sample_interval", parse_kfence_early_init);
> +#endif

AFAICS except for kfence_disabled, this can all go in radix_pgtable.c.

That would make it unambiguous that kfence_early_init is only used by
the radix kfence code, and avoid any bad interactions with the other
subarches - which was my original concern about adding the code here.

cheers