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
