On Thu, Nov 3, 2022 at 11:15 AM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
On Thu, Nov 3, 2022 at 4:23 AM Russell King (Oracle) linux@armlinux.org.uk wrote:
On Thu, Nov 03, 2022 at 05:21:16PM +0800, Yang Jihong wrote:
The error code -EACCES is returned when bpf prog is tested in 32-bit environment, This is because bpf_object__relocate modifies the instruction to change memory size to 4 bytes, as shown in the following messages:
libbpf: prog 'kfunc_call_test1': relo #2: matching candidate #0 <byte_off> [18342] struct __sk_buff.sk (0:30:0 @ offset 168) libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) off 168 -> 168 libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) mem_sz 8 -> 4
As a result, the bpf_skb_is_valid_access check fails. For 32-bit architecture, unnecessary checks need to be deleted.
Isn't the purpose of this check to ensure that the entire pointer is written, and BPF can't write half of it?
case offsetof(struct __sk_buff, sk):
if (type == BPF_WRITE || size != sizeof(__u64))return false;Wouldn't "(size != sizeof(struct bpf_sock *) && size != sizeof(__u64))" be more appropriate here, so 32-bit can only write the 32-bit pointer or the full 64-bit value, and 64-bit can only write the 64-bit pointer? Or is there a reason not to? bpf folk?
You're correct. The patch is completely wrong. The bug is elsewhere.
So I looked at this a bit (and replied to the old version of this patch). What happens in the kernel is that we expect 64-bit load but rewrite it to 32-bit load on 32-bit architectures (because we just use sizeof(struct sk_buff, sk) which is 4 bytes on 32-bit arch.
The problem here is that libbpf adjusts such pointer accesses from 8-byte read to 4-byte reads for preserve_access_index (because libbpf sees that pointer is really 4 byte long), which is what we actually want in the general case. Here the assumption was made before CO-RE that __sk_buff is a stable (and fake) UAPI and the correct BPF program will access sk as a 64-bit pointer because BPF-side pointers always appear as 64-bit.
But from a correctness standpoint I think it should be fine to enable both 32- and 64-bit loads for such pointers in __sk_buff for 32-bit host arch. This will work well with CO-RE and will be correctly rewritten to 32-bit or 64-bit accesses, depending on host architecture.
We should still reject 32-bit load on 64-bit host arch, though.