Re: [PATCH bpf-next 11/16] bpf, arm64: Add support for arena percpu

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf, arm64: upper 32 bits not cleared for arena percpu in build_insn()
- [Low] bpf, arm64: link failure in build_insn() when CONFIG_BPF_SYSCALL=n
--

commit 06cd99f2557b7d24037d63a9c40cf67ca1795637
Author: Xu Kuohai <xukuohai@huawei.com>

bpf, arm64: Add support for arena percpu

This commit adds arm64 JIT support for the BPF_PSEUDO_ARENA_PERCPU pseudo
instruction by computing smp_processor_id() * chunk_sz and adding it to CPU
0's arena address.

> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 1d5f4cf88d331..998806cc700eb 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c

[ ... ]

> @@ -1829,6 +1835,26 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn
>  		else
>  			emit_a64_mov_i64(dst, imm64, ctx);
>  
> +#ifdef CONFIG_SMP
> +		if (insn->src_reg == BPF_PSEUDO_ARENA_PERCPU) {
> +			u32 chunk_sz = bpf_arena_percpu_chunk_sz(bpf_prog_arena(ctx->prog));

[Severity: Low]
Will linking vmlinux fail with undefined references to bpf_prog_arena() and
bpf_arena_percpu_chunk_sz() when building with CONFIG_BPF_JIT=y,
CONFIG_SMP=y, and CONFIG_BPF_SYSCALL=n?

arch/arm64/net/Makefile compiles bpf_jit_comp.o whenever CONFIG_BPF_JIT=y,
whereas kernel/bpf/Makefile only builds arena.o when CONFIG_BPF_SYSCALL=y:

kernel/bpf/Makefile:
    ifeq ($(CONFIG_MMU)$(CONFIG_64BIT),yy)
    obj-$(CONFIG_BPF_SYSCALL) += arena.o range_tree.o
    endif

Other arena helpers used by the JIT, such as bpf_arena_get_user_vm_start() and
bpf_arena_get_kern_vm_start(), have __weak fallbacks in kernel/bpf/core.c or
inline stubs in include/linux/bpf.h, but bpf_prog_arena() and
bpf_arena_percpu_chunk_sz() are only defined in kernel/bpf/arena.c.

> +
> +			/* tmp = smp_processor_id() */
> +			emit_current_cpu_id(tmp, ctx);
> +			/*
> +			 * tmp *= chunk_sz
> +			 * arena_map_alloc() guarantees
> +			 * chunk_sz * nr_cpu_ids <= vm_range <= 4 GiB and
> +			 * cpu_number < nr_cpu_ids, so a 32-bit mul
> +			 * cannot overflow here.
> +			 */
> +			emit_a64_mov_i(0, tmp2, chunk_sz, ctx);
> +			emit(A64_MUL(0, tmp, tmp, tmp2), ctx);
> +			/* dst += tmp */
> +			emit(A64_ADD(1, dst, dst, tmp), ctx);
> +		}
> +#endif
> +
>  		return 1;
>  	}

[Severity: Critical]
Can this leave non-zero upper 32 bits in dst when arena->user_vm_start is at
or above 4 GiB, causing subsequent arena memory operations on dst to access
out-of-bounds kernel virtual memory?

In kernel/bpf/verifier.c:check_and_resolve_insns(), BPF_PSEUDO_ARENA_PERCPU
is resolved via arena_map_direct_value_addr() to the full 64-bit
arena->user_vm_start + off:

    err = map->ops->map_direct_value_addr(map, &addr, off);
    ...
    aux->map_off = off;
    addr += off;
    ...
    insn[0].imm = (u32)addr;
    insn[1].imm = addr >> 32;

Unlike BPF_PSEUDO_MAP_VALUE (which marks the register as SCALAR_VALUE so that
dereferencing it requires an addr_space_cast or scalar access fixup that
truncates the value to 32 bits), check_ld_imm() marks the destination register
of BPF_PSEUDO_ARENA_PERCPU directly as PTR_TO_ARENA:

    if (insn->src_reg == BPF_PSEUDO_ARENA_PERCPU) {
        u64 pos = aux->map_off - bpf_arena_percpu_chunk_off(map);
        ...
        __mark_reg_known(dst_reg, pos);
        dst_reg->type = PTR_TO_ARENA;
        dst_reg->mem_size = bpf_arena_percpu_chunk_sz(map);
    }

Because dst_reg is already PTR_TO_ARENA, it can be dereferenced directly by
BPF_LDX, BPF_STX, BPF_ST, or BPF_ATOMIC instructions without an intervening
addr_space_cast. In arch/arm64/net/bpf_jit_comp.c:build_insn(), the
BPF_PROBE_MEM32, BPF_PROBE_MEM32SX, and BPF_PROBE_ATOMIC handlers assume the
upper 32 bits of a PTR_TO_ARENA register are already cleared and perform an
unmasked 64-bit addition with arena_vm_base (kern_vm_start):

    if (BPF_MODE(insn->code) == BPF_PROBE_MEM32 ||
        BPF_MODE(insn->code) == BPF_PROBE_MEM32SX) {
        emit(A64_ADD(1, tmp2, src, arena_vm_base), ctx);
        src = tmp2;
    }

However, for BPF_PSEUDO_ARENA_PERCPU, build_insn() loads the full 64-bit imm64
via emit_a64_mov_i64(dst, imm64, ctx) and then either adds tmp with a 64-bit
A64_ADD(1, dst, dst, tmp) on CONFIG_SMP or leaves dst untouched on
!CONFIG_SMP. In both cases, bits [63:32] of dst retain user_vm_start >> 32, so
dereferencing dst accesses kern_vm_start + user_vm_start + off + cpu_off
instead of kern_vm_start + (u32)(user_vm_start + off + cpu_off).

Should BPF_PSEUDO_ARENA_PERCPU zero-extend the 32-bit address on both
CONFIG_SMP and !CONFIG_SMP (for example, loading only the 32-bit imm and using
a 32-bit A64_ADD(0, dst, dst, tmp))?

[ ... ]

>
>-- 
>2.39.5
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792503001.git.xukuohai@huawei.com?part=11