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

On 8/11/26 4:27 PM, [email protected] wrote:
[...]
>> 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?

Hm, so the __arch gate is forced by __stderr() and bpf_prog_report_arena_violation()
currently only has callers in the x86-64 and arm64 ex handlers. I'd leave
as-is for now.

[...]
> 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.

For the sake of what we test here, I'd leave as-is since the fault type
doesn't really matter here but rather the fact that we match on arena WRITE
plus the register clear.

> [ ... ]
> 
>> +	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.
Hm, not relevant for upstream BPF CI given green as can be seen, but I can
look into that if desired.