Re: [PATCH bpf-next v3 6/6] selftests/bpf: Add arena fault tests for atomics with fetch

> diff --git a/tools/testing/selftests/bpf/prog_tests/stream.c b/tools/testing/selftests/bpf/prog_tests/stream.c
> --- a/tools/testing/selftests/bpf/prog_tests/stream.c
> +++ b/tools/testing/selftests/bpf/prog_tests/stream.c
> @@ -105,6 +105,10 @@ void test_stream_arena_fault_address(void)
>  		test_address(skel->progs.stream_arena_write_fault, &skel->bss->fault_addr);
>  	if (test__start_subtest("load_acquire_fault"))
>  		test_address(skel->progs.stream_arena_load_acquire_fault, &skel->bss->fault_addr);
> +	if (test__start_subtest("xchg_fault"))
> +		test_address(skel->progs.stream_arena_xchg_fault, &skel->bss->fault_addr);
> +	if (test__start_subtest("cmpxchg_fault"))
> +		test_address(skel->progs.stream_arena_cmpxchg_fault, &skel->bss->fault_addr);
>
>  	stream__destroy(skel);
>  }
> diff --git a/tools/testing/selftests/bpf/progs/stream.c b/tools/testing/selftests/bpf/progs/stream.c
> --- a/tools/testing/selftests/bpf/progs/stream.c
> +++ b/tools/testing/selftests/bpf/progs/stream.c
> @@ -229,6 +229,107 @@ int stream_arena_load_acquire_fault(void *ctx)
>  	return val;
>  }
>
> +SEC("syscall")
> +__arch_x86_64
> +__arch_arm64
> +__success __retval(0)
> +__stderr("ERROR: Arena WRITE access at unmapped address 0x{{.*}}")

The series includes fixes for four JITs (x86, arm64, riscv, s390), but
these new programs only cover two of them. Could __arch_riscv64 and
__arch_s390x variants be added for the other half?

The riscv64 and s390 JIT fixes exist in the same series:

  a49d067b71354 bpf, riscv: Clear fetch destination on faulting arena atomic
  e0de31f547978 bpf, s390:  Clear fetch destination on faulting arena atomic

but the __arch gate provably excludes them from these tests.

The __stderr() half unavoidably gates on x86_64 and arm64, since only
those two JITs call bpf_prog_report_arena_violation(). Both the riscv
and s390 changelogs state this explicitly.

But the __retval(0) half does not depend on arena violation reporting.
A variant tagged __arch_riscv64 __arch_s390x with __success __retval(0)
and no __stderr() would exercise exactly the register-clear fix those two
commits make, using the same poison-and-return mechanism. As written, two
of the four fixed JITs ship with no test coverage.

> +__stderr("CPU: {{[0-9]+}} UID: 0 PID: {{[0-9]+}} Comm: {{.*}}")
> +__stderr("Call trace:\n"
> +"{{([a-zA-Z_][a-zA-Z0-9_]*\\+0x[0-9a-fA-F]+/0x[0-9a-fA-F]+\n"
> +"|[ \t]+[^\n]+\n)*}}")
> +int stream_arena_xchg_fault(void *ctx)
> +{
> +	static const struct bpf_insn xchg_insn = {
> +		.code	 = 0xc3,	/* BPF_STX | BPF_ATOMIC | BPF_W */
> +		.dst_reg = 1,		/* BPF_REG_1 */
> +		.src_reg = 2,		/* BPF_REG_2 */
> +		.off	 = 0x7fff,
> +		.imm	 = 0xe1,	/* BPF_XCHG */
> +	};

Does the misaligned .off = 0x7fff cause the test to validate a different
fault path on arm64 than the commit message describes?

user_vm_start is page aligned, so the effective address is
user_vm_start + 0x7fff, which is 3 mod 4. The 4-byte BPF_W access also
straddles the page boundary at 0x8000.

For plain LDR/STR (stream_arena_read_fault, stream_arena_write_fault),
this offset works as intended: arm64 permits unaligned access with
SCTLR_EL1.A=0, so those really do take a translation fault on the unmapped
page.

But atomics require natural alignment. arm64 LSE CAS/SWP raise an EL1
Alignment fault (ESR FSC 0b100001) when the access is misaligned or crosses
a 16-byte granule, and that takes priority over the translation fault.

The test still passes - fixup_exception() in arch/arm64/mm/fault.c runs
before any die_kernel_fault path, so ex_handler_bpf() still clears the
fetch register and still prints "Arena WRITE access at unmapped address".
The fix under test is fault-type independent.

But the arm64 arm of both new subtests validates the alignment-fault path
rather than the unmapped-arena-page path, while x86-64 (where page fault
is delivered ahead of split-lock alignment check) validates the intended
one, so the two architectures end up testing different code paths.

A 4-byte-aligned offset inside the unmapped page (e.g. 0x7ffc) would keep
the fault a translation fault on both architectures. The pre-existing
stream_arena_load_acquire_fault has the same property with its 0x7fff.

[ ... ]

> +	return val;
> +}
> +
> +SEC("syscall")
> +__arch_x86_64
> +__arch_arm64

A subsystem pattern flags this as potentially concerning: On arm64 these
two programs only load on CPUs that implement LSE atomics, and nothing in
the test gates on that.

arch/arm64/net/bpf_jit_comp.c:bpf_jit_supports_insn() rejects arena RMW
atomics without LSE:

  bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena)
  {
  	if (!in_arena)
  		return true;
  	switch (insn->code) {
  	case BPF_STX | BPF_ATOMIC | BPF_W:
  	case BPF_STX | BPF_ATOMIC | BPF_DW:
  		if (!bpf_atomic_is_load_store(insn) &&
  		    !cpus_have_cap(ARM64_HAS_LSE_ATOMICS))
  			return false;
  	}
  	return true;
  }

The verifier consults it for arena destinations in atomic_ptr_type_ok()
and on failure rejects the program with "BPF_ATOMIC stores into R1 arena
is not allowed". BPF_XCHG (0xe1) and BPF_CMPXCHG (0xf1) are not load-store
forms, so bpf_atomic_is_load_store() returns false for both and the LSE
check applies.

This is new for this object. Before the patch the arena programs in
progs/stream.c used plain BPF_LDX/BPF_STX and one BPF_LOAD_ACQ, and
BPF_LOAD_ACQ is exempted by bpf_atomic_is_load_store(). progs/stream.c
therefore had no CPU-feature dependency until now.

On an ARMv8.0 core (Cortex-A53/A57/A72) the consequences reach beyond the
two new subtests, because stream__open_and_load() is an all-or-nothing
whole-object load:

  - test_stream_arena_fault_address(): stream__open_and_load() returns
    NULL, ASSERT_OK_PTR fails and the function returns, so read_fault,
    write_fault and load_acquire_fault are lost too - three subtests
    that pass today.
  - test_stream_syscall(): same whole-object load, and this test is
    otherwise entirely architecture-neutral.
  - test_stream_success() (RUN_TESTS) loads programs individually
    (bpf_program__set_autoload), so there the damage is limited to the
    two new programs reporting load failure.

Would a runtime LSE probe, or moving the two RMW programs into a separate
object so a load failure cannot take down the unrelated subtests, contain
this?

progs/arena_atomics.c already does arena xchg/cmpxchg guarded only by
compile-time checks, prog_tests/arena_atomics.c loads that whole object
with arena_atomics__load(), and DENYLIST.aarch64 is empty - so the
arena_atomics test presumably already fails on non-LSE arm64. This class
of gap is pre-existing in the tree.

> +__success __retval(0)
> +__stderr("ERROR: Arena WRITE access at unmapped address 0x{{.*}}")

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31496528509