Re: [PATCH v2 3/3] riscv: keep booting if KFENCE pool reservation fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] riscv: kfence: incomplete replacement pool rejection and fallback memblock leak
--

commit 3fb224ed3ecf516e32ff76314492cf99262a5a4f
Author: Jackie Liu <liuyun01@kylinos.cn>

riscv: keep booting if KFENCE pool reservation fails

This commit avoids hitting BUG_ON() during linear mapping setup when the
early KFENCE pool reservation fails by tracking the failure in
kfence_disabled and returning !kfence_disabled from arch_kfence_init_pool().

> diff --git a/arch/riscv/include/asm/kfence.h b/arch/riscv/include/asm/kfence.h
> index 29cb3a6ee113d..57e57426a7d60 100644
> --- a/arch/riscv/include/asm/kfence.h
> +++ b/arch/riscv/include/asm/kfence.h
> @@ -9,9 +9,11 @@
>  #include <asm/cacheflush.h>
>  #include <asm/pgtable.h>
>  
> +extern bool kfence_disabled;
> +
>  static inline bool arch_kfence_init_pool(void)
>  {
> -	return true;
> +	return !kfence_disabled;
>  }
>  
>  static inline bool kfence_protect_page(unsigned long addr, bool protect)
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index fb37b0b67efec..d21be990346c6 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -44,6 +44,10 @@ DECLARE_BITMAP(new_valid_map_cpus, NR_CPUS);
>  struct kernel_mapping kernel_map __ro_after_init;
>  EXPORT_SYMBOL(kernel_map);
>  
> +#ifdef CONFIG_KFENCE
> +bool kfence_disabled __ro_after_init;
> +#endif
> +
>  #ifdef CONFIG_64BIT
>  u64 satp_mode __ro_after_init = SATP_MODE_57;
>  #else
> @@ -1229,10 +1233,13 @@ static void __init create_linear_mapping_page_table(void)
>  	 *  for this region.
>  	 */
>  	kfence_pool = memblock_phys_alloc(KFENCE_POOL_SIZE, PAGE_SIZE);
> -	BUG_ON(!kfence_pool);
> -
> -	memblock_mark_nomap(kfence_pool, KFENCE_POOL_SIZE);
> -	__kfence_pool = __va(kfence_pool);
> +	if (kfence_pool) {
> +		memblock_mark_nomap(kfence_pool, KFENCE_POOL_SIZE);
> +		__kfence_pool = __va(kfence_pool);
> +	} else {
> +		pr_err("failed to allocate kfence pool\n");
> +		kfence_disabled = true;
> +	}

[Severity: High]
Does setting kfence_disabled without clearing kfence_sample_interval still
allow a fallback memblock allocation and a late hugepage-backed replacement
pool?

In mm/kfence/core.c:kfence_alloc_pool_and_metadata():

	if (!kfence_sample_interval)
		return;

	/*
	 * If the pool has already been initialized by arch, there is no need to
	 * re-allocate the memory pool.
	 */
	if (!__kfence_pool)
		__kfence_pool = memblock_alloc(KFENCE_POOL_SIZE, PAGE_SIZE);
	...
	kfence_metadata_init = memblock_alloc(KFENCE_METADATA_SIZE, PAGE_SIZE);
	if (!kfence_metadata_init) {
		pr_err("failed to allocate metadata\n");
		memblock_free(__kfence_pool, KFENCE_POOL_SIZE);
		__kfence_pool = NULL;
	}

There look to be two issues with relying solely on kfence_disabled here:

First, when memblock_phys_alloc() fails in create_linear_mapping_page_table(),
__kfence_pool remains NULL while kfence_sample_interval is still non-zero.
Later in mm_core_init(), kfence_alloc_pool_and_metadata() will allocate a
fallback __kfence_pool and kfence_metadata_init from memblock. When
kfence_init() -> kfence_init_pool_early() subsequently runs and sees
arch_kfence_init_pool() return false, it calls memblock_free() on both
allocations after kmem_cache_init() has already set slab_is_available().
When CONFIG_DEFERRED_STRUCT_PAGE_INIT is enabled, memblock_free() ->
memblock_phys_free() -> __free_reserved_area() triggers a warning because
deferred struct page initialization has not yet run, leaking both
__kfence_pool and kfence_metadata_init.

Second, if memblock_phys_alloc() succeeds in
create_linear_mapping_page_table() (leaving kfence_disabled false), but the
subsequent kfence_metadata_init allocation in kfence_alloc_pool_and_metadata()
fails, the early PTE-mapped __kfence_pool is freed and reset to NULL while
kfence_disabled remains false. If KFENCE is later enabled at runtime via
sample_interval, kfence_enable_late() -> kfence_init_late() will allocate a
replacement pool from the linear map (which may be backed by PMD/PUD/P4D/PGD
hugepages), arch_kfence_init_pool() will return true, and
kfence_protect_page() -> virt_to_kpte() will walk the page tables assuming
PTE mappings, which can corrupt memory or fault.

Would setting kfence_sample_interval = 0 on allocation failure in
create_linear_mapping_page_table() and checking whether the address falls
within the early PTE-mapped pool range (or disabling late initialization in
arch_kfence_init_pool()) avoid both paths?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793242457.git.liuyun01@kylinos.cn?part=3