Re: [RFC PATCH 5/9] arm64: mm: Permit permissions changes on huge vmappings

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Live splitting of huge vmalloc mappings silently fails and triggers a kernel warning.
- [High] Permission changes on small, PTE-mapped vmalloc areas are incorrectly rejected.
- [High] Improper use of `WARN_ON_ONCE` for a hardware capability check causes unwarranted kernel log spam and potential panic.
- [Low] Implicit declaration of function `system_supports_bbml2_noabort()` breaks the build.
--

--- Patch [5]: [RFC PATCH 5/9] arm64: mm: Permit permissions changes on huge vmappings ---
commit e67f32e911a059a6f372cf2818088d9fde53ee7f
Author: Ard Biesheuvel <[email protected]>

arm64: mm: Permit permissions changes on huge vmappings

This commit allows permission changes on huge vmappings without splitting
when the region is PMD-aligned or when the system supports live splitting.

> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> index bbe98ac9ad8c6..20ff9cb273c11 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c

[ ... ]

> @@ -179,7 +177,16 @@ static int change_memory_common(unsigned long addr, int numpages,
>  	if (!area ||
>  	    ((unsigned long)kasan_reset_tag((void *)end) >
>  	     (unsigned long)kasan_reset_tag(area->addr) + area->size) ||
> -	    ((area->flags & (VM_ALLOC | VM_ALLOW_HUGE_VMAP)) != VM_ALLOC))
> +	    !(area->flags & VM_ALLOC))
>                 return -EINVAL;
>  
> +	/*
> +	 * Disallow VM_ALLOW_HUGE_VMAP mappings unless the region is PMD
> +	 * aligned, or splitting live huge mappings is supported.
> +	 */
> +	if ((area->flags & VM_ALLOW_HUGE_VMAP) &&
> +	   ((start % PMD_SIZE) || (size % PMD_SIZE)) &&
> +	   WARN_ON_ONCE(!system_supports_bbml2_noabort()))

[Severity: Low]
Does the function system_supports_bbml2_noabort() exist in the codebase? It
appears to be missing a definition or declaration, which might cause the
kernel to fail to compile due to an implicit function declaration.

[Severity: High]
Is WARN_ON_ONCE() the right choice for a hardware capability check here?

If an unaligned permission change is requested on an older system lacking
BBML2 support, this will trigger a warning stack trace. If panic_on_warn
is enabled, an unprivileged user loading a BPF program could trigger a
kernel panic. Should this condition fail silently instead?

[Severity: High]
Does checking area->flags & VM_ALLOW_HUGE_VMAP incorrectly reject small,
PTE-mapped vmalloc areas?

Allocators like execmem use VM_ALLOW_HUGE_VMAP as an allocation hint. If
an allocation is smaller than PMD_SIZE, it falls back to PTE mappings but
retains the VM_ALLOW_HUGE_VMAP flag in area->flags.

When change_memory_common() processes this on older hardware without
BBML2, it sees the flag alongside an unaligned size, and rejects the
request with -EINVAL, even though the page table is already PTE-mapped
and requires no splitting.

Should we check the actual page table granularity, such as inspecting
area->page_order, instead of just the allocation flag?

[Severity: High]
If this condition passes because system_supports_bbml2_noabort() is true,
will the PMD actually be split later?

change_memory_common() delegates to __change_memory_common() which
calls update_range_prot() and eventually split_kernel_leaf_mapping().

Looking at split_kernel_leaf_mapping() in arch/arm64/mm/mmu.c:

if (!linear_map_requires_bbml3 || is_kfence_address((void *)start))
    return 0;

if (!system_supports_bbml3()) {
    ...
    return 0;
}

This evaluates system_supports_bbml3() and linear_map_requires_bbml3,
ignoring both the BBML2 capability and the vmalloc context. If those are
false (for example, if rodata is off), it returns 0 without splitting the
PMD.

The subsequent lockless page table walk would then encounter an unsplit PMD
spanning a larger range than requested, resulting in a warning and a
failure.

> +		return -EINVAL;
>  
>  	if (!numpages)
>  		return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5