Re: [PATCH bpf-next v2 4/5] bpf, x86: make sure allocation in arch_bpf_trampoline_size() is writable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Unnecessary page table modifications and hugepage shattering during trampoline size calculation.

Pre-existing issues:
- [High] Ignoring `execmem_restore_rox` failure leads to NX memory being added to the ROX cache.
--

--- Patch [4]: [PATCH bpf-next v2 4/5] bpf, x86: make sure allocation in arch_bpf_trampoline_size() is writable ---
commit 837979675b78fb56749ec25f57d0084b1bf805ee
Author: Mike Rapoport (Microsoft) <rppt@kernel.org>

bpf, x86: make sure allocation in arch_bpf_trampoline_size() is writable
    
arch_bpf_trampoline_size() allocates a buffer to get actual size required
for a trampoline.
    
This buffer must be in the module address space because
__arch_prepare_bpf_trampoline() calculates rel32 offsets relatively to
that buffer.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index de7515ea1beae..b2feec81e2312 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -3703,13 +3703,12 @@ int arch_bpf_trampoline_size(const struct btf_func_model *m, u32 flags,
>  	int ret;
>  
>  	/* Allocate a temporary buffer for __arch_prepare_bpf_trampoline().
> -	 * This will NOT cause fragmentation in direct map, as we do not
> -	 * call set_memory_*() on this buffer.
>  	 *
>  	 * We cannot use kvmalloc here, because we need image to be in
>  	 * module memory range.
> +	 * Since it must be writable use bpf_jit_alloc_exec_rw().
>  	 */
> -	image = bpf_jit_alloc_exec(PAGE_SIZE);
> +	image = bpf_jit_alloc_exec_rw(PAGE_SIZE);

[Severity: Medium]
Does this allocation cause unnecessary page table modifications and hugepage 
shattering?

Allocating via bpf_jit_alloc_exec_rw() forces page table permission changes
(set_memory_nx and set_memory_rw), which triggers cross-CPU TLB flushes
and permanently splits 2MB hugepages in the ROX cache's direct map. When
the buffer is immediately freed, these operations are repeated.

Since arch_bpf_trampoline_size() only needs a dummy pointer in the module
memory range to satisfy relative jump offset checks during instruction
emission, could we pass func_addr as the dummy image pointer and use 
kvmalloc() for rw_image instead?

>  	if (!image)
>  		return -ENOMEM;
>  

[ ... ]

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 1b89c18cf2461..e2076667b2451 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1128,6 +1128,11 @@ void *bpf_jit_alloc_exec(unsigned long size)
>  	return execmem_alloc(EXECMEM_BPF, size);
>  }
>  
> +void *bpf_jit_alloc_exec_rw(unsigned long size)
> +{
> +	return execmem_alloc_rw(EXECMEM_BPF, size);
> +}
> +
>  void bpf_jit_free_exec(void *addr)
>  {
>  	execmem_free(addr);

[Severity: High]
This is a pre-existing issue, but there appears to be a flaw when freeing
memory to the ROX cache in __execmem_cache_free():

mm/execmem.c:__execmem_cache_free() {
...
	execmem_fill_trapping_insns(ptr, size);
	execmem_restore_rox(ptr, size);

	err = execmem_cache_add_locked(ptr, size, gfp_mask);
...
}

Does this ignore the return value of execmem_restore_rox()?

If execmem_restore_rox() fails (e.g., due to OOM when allocating page
tables), the memory remains Non-Executable (NX). Because the return value
is ignored, the NX memory is unconditionally added back to the ROX cache.

When subsequent allocations return this NX memory and the BPF JIT attempts
to execute code from it, will it trigger a fatal page fault?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-execmem-x86-rox-bpf-v0-v2-0-bfd956d35119@kernel.org?part=4