arm64: mm: Simplify check in arch_kfence_init_pool()

[PATCH] arm64: mm: Simplify check in arch_kfence_init_pool()

Posted by Kevin Brodsky 1 week, 5 days ago

TL;DR: checking force_pte_mapping() in arch_kfence_init_pool() is
sufficient

Commit ce2b3a50ad92 ("arm64: mm: Don't sleep in
split_kernel_leaf_mapping() when in atomic context") recently added
an arm64 implementation of arch_kfence_init_pool() to ensure that
the KFENCE pool is PTE-mapped. Assuming that the pool was not
initialised early, block splitting is necessary if the linear
mapping is not fully PTE-mapped, in other words if
force_pte_mapping() is false.

arch_kfence_init_pool() currently makes another check: whether
BBML2-noabort is supported, i.e. whether we are *able* to split
block mappings. This check is however unnecessary, because
force_pte_mapping() is always true if KFENCE is enabled and
BBML2-noabort is not supported. This must be the case by design,
since KFENCE requires PTE-mapped pages in all cases. We can
therefore remove that check.

The situation is different in split_kernel_leaf_mapping(), as that
function is called unconditionally regardless of the configuration.
If BBML2-noabort is not supported, it cannot do anything and bails
out. If force_pte_mapping() is true, there is nothing to do and it
also bails out, but these are independent checks.

Commit 53357f14f924 ("arm64: mm: Tidy up force_pte_mapping()")
grouped these checks into a helper, split_leaf_mapping_possible().
This isn't so helpful as only split_kernel_leaf_mapping() should
check both. Revert the parts of that commit that introduced the
helper, reintroducing the more accurate comments in
split_kernel_leaf_mapping().

Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
---
Apologies for not suggesting this during the review cycle of [1], I'm
late to the party...

[1] https://lore.kernel.org/all/20251106160945.3182799-1-ryan.roberts@arm.com/
---
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Christoph Lameter (Ampere) <cl@gentwo.org>
Cc: David Hildenbrand <david@redhat.com>
Cc: Dev Jain <dev.jain@arm.com>
Cc: D Scott Phillips <scott@os.amperecomputing.com>
Cc: Ryan Roberts <ryan.roberts@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Yang Shi <yang@os.amperecomputing>
---
 arch/arm64/mm/mmu.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 2ba01dc8ef82..866cc889bb61 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -720,18 +720,6 @@ static inline bool force_pte_mapping(void)
 	return rodata_full || arm64_kfence_can_set_direct_map() || is_realm_world();
 }
 
-static inline bool split_leaf_mapping_possible(void)
-{
-	/*
-	 * !BBML2_NOABORT systems should never run into scenarios where we would
-	 * have to split. So exit early and let calling code detect it and raise
-	 * a warning.
-	 */
-	if (!system_supports_bbml2_noabort())
-		return false;
-	return !force_pte_mapping();
-}
-
 static DEFINE_MUTEX(pgtable_split_lock);
 
 int split_kernel_leaf_mapping(unsigned long start, unsigned long end)
@@ -739,11 +727,22 @@ int split_kernel_leaf_mapping(unsigned long start, unsigned long end)
 	int ret;
 
 	/*
-	 * Exit early if the region is within a pte-mapped area or if we can't
-	 * split. For the latter case, the permission change code will raise a
-	 * warning if not already pte-mapped.
+	 * !BBML2_NOABORT systems should not be trying to change permissions on
+	 * anything that is not pte-mapped in the first place. Just return early
+	 * and let the permission change code raise a warning if not already
+	 * pte-mapped.
 	 */
-	if (!split_leaf_mapping_possible() || is_kfence_address((void *)start))
+	if (!system_supports_bbml2_noabort())
+		return 0;
+
+	/*
+	 * If the region is within a pte-mapped area, there is no need to try to
+	 * split. Additionally, CONFIG_DEBUG_PAGEALLOC and CONFIG_KFENCE may
+	 * change permissions from atomic context so for those cases (which are
+	 * always pte-mapped), we must not go any further because taking the
+	 * mutex below may sleep.
+	 */
+	if (force_pte_mapping() || is_kfence_address((void *)start))
 		return 0;
 
 	/*
@@ -1042,7 +1041,7 @@ bool arch_kfence_init_pool(void)
 	int ret;
 
 	/* Exit early if we know the linear map is already pte-mapped. */
-	if (!split_leaf_mapping_possible())
+	if (force_pte_mapping())
 		return true;
 
 	/* Kfence pool is already pte-mapped for the early init case. */

base-commit: 6a23ae0a96a600d1d12557add110e0bb6e32730c
-- 
2.51.2

Re: [PATCH] arm64: mm: Simplify check in arch_kfence_init_pool()

Posted by Catalin Marinas 1 week ago

On Wed, Nov 19, 2025 at 01:00:16PM +0000, Kevin Brodsky wrote:
> TL;DR: checking force_pte_mapping() in arch_kfence_init_pool() is
> sufficient

Thanks Kevin. I'll push it at -rc1 since it depends on commits merged in
6.18-rc6 and I don't want to rebase my for-next branches now.

-- 
Catalin

Re: [PATCH] arm64: mm: Simplify check in arch_kfence_init_pool()

Posted by Ryan Roberts 1 week, 5 days ago

On 19/11/2025 13:00, Kevin Brodsky wrote:
> TL;DR: checking force_pte_mapping() in arch_kfence_init_pool() is
> sufficient
> 
> Commit ce2b3a50ad92 ("arm64: mm: Don't sleep in
> split_kernel_leaf_mapping() when in atomic context") recently added
> an arm64 implementation of arch_kfence_init_pool() to ensure that
> the KFENCE pool is PTE-mapped. Assuming that the pool was not
> initialised early, block splitting is necessary if the linear
> mapping is not fully PTE-mapped, in other words if
> force_pte_mapping() is false.
> 
> arch_kfence_init_pool() currently makes another check: whether
> BBML2-noabort is supported, i.e. whether we are *able* to split
> block mappings. This check is however unnecessary, because
> force_pte_mapping() is always true if KFENCE is enabled and
> BBML2-noabort is not supported. This must be the case by design,
> since KFENCE requires PTE-mapped pages in all cases. We can
> therefore remove that check.
> 
> The situation is different in split_kernel_leaf_mapping(), as that
> function is called unconditionally regardless of the configuration.
> If BBML2-noabort is not supported, it cannot do anything and bails
> out. If force_pte_mapping() is true, there is nothing to do and it
> also bails out, but these are independent checks.
> 
> Commit 53357f14f924 ("arm64: mm: Tidy up force_pte_mapping()")
> grouped these checks into a helper, split_leaf_mapping_possible().
> This isn't so helpful as only split_kernel_leaf_mapping() should
> check both. Revert the parts of that commit that introduced the
> helper, reintroducing the more accurate comments in
> split_kernel_leaf_mapping().
> 
> Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>

I agree that it's clearer like this. Nice tidy up - thanks for doing it.

Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>

> ---
> Apologies for not suggesting this during the review cycle of [1], I'm
> late to the party...
> 
> [1] https://lore.kernel.org/all/20251106160945.3182799-1-ryan.roberts@arm.com/
> ---
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Christoph Lameter (Ampere) <cl@gentwo.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Dev Jain <dev.jain@arm.com>
> Cc: D Scott Phillips <scott@os.amperecomputing.com>
> Cc: Ryan Roberts <ryan.roberts@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Yang Shi <yang@os.amperecomputing>
> ---
>  arch/arm64/mm/mmu.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 2ba01dc8ef82..866cc889bb61 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -720,18 +720,6 @@ static inline bool force_pte_mapping(void)
>  	return rodata_full || arm64_kfence_can_set_direct_map() || is_realm_world();
>  }
>  
> -static inline bool split_leaf_mapping_possible(void)
> -{
> -	/*
> -	 * !BBML2_NOABORT systems should never run into scenarios where we would
> -	 * have to split. So exit early and let calling code detect it and raise
> -	 * a warning.
> -	 */
> -	if (!system_supports_bbml2_noabort())
> -		return false;
> -	return !force_pte_mapping();
> -}
> -
>  static DEFINE_MUTEX(pgtable_split_lock);
>  
>  int split_kernel_leaf_mapping(unsigned long start, unsigned long end)
> @@ -739,11 +727,22 @@ int split_kernel_leaf_mapping(unsigned long start, unsigned long end)
>  	int ret;
>  
>  	/*
> -	 * Exit early if the region is within a pte-mapped area or if we can't
> -	 * split. For the latter case, the permission change code will raise a
> -	 * warning if not already pte-mapped.
> +	 * !BBML2_NOABORT systems should not be trying to change permissions on
> +	 * anything that is not pte-mapped in the first place. Just return early
> +	 * and let the permission change code raise a warning if not already
> +	 * pte-mapped.
>  	 */
> -	if (!split_leaf_mapping_possible() || is_kfence_address((void *)start))
> +	if (!system_supports_bbml2_noabort())
> +		return 0;
> +
> +	/*
> +	 * If the region is within a pte-mapped area, there is no need to try to
> +	 * split. Additionally, CONFIG_DEBUG_PAGEALLOC and CONFIG_KFENCE may
> +	 * change permissions from atomic context so for those cases (which are
> +	 * always pte-mapped), we must not go any further because taking the
> +	 * mutex below may sleep.
> +	 */
> +	if (force_pte_mapping() || is_kfence_address((void *)start))
>  		return 0;
>  
>  	/*
> @@ -1042,7 +1041,7 @@ bool arch_kfence_init_pool(void)
>  	int ret;
>  
>  	/* Exit early if we know the linear map is already pte-mapped. */
> -	if (!split_leaf_mapping_possible())
> +	if (force_pte_mapping())
>  		return true;
>  
>  	/* Kfence pool is already pte-mapped for the early init case. */
> 
> base-commit: 6a23ae0a96a600d1d12557add110e0bb6e32730c