Hello,
On 2022/11/5 7:37, Alexei Starovoitov wrote:
On Fri, Nov 4, 2022 at 3:43 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
On Thu, Nov 3, 2022 at 11:15 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
On Thu, Nov 3, 2022 at 4:23 AM Russell King (Oracle)
<linux@xxxxxxxxxxxxxxx> 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.
Replied in the other thread as well :)
The CO_RE screws up access here.
Since it's a load of a pointer the verifier has to see it as a 8-byte load.
When CO-RE converts it to 4 byte the verifier won't recognize it
as a pointer load anymore.
We cannot easily convert 64-bit BPF ISA into 32-bit.
It's a massive amount of work.
.
From the above discussion, there are two different solutions:
1. Modify bpf_skb_is_valid_access to ensure that 32-bit can only load
the 32-bit pointer or the full 64-bit value, and 64-bit can only load
the 64-bit pointer
2. Modify libbpf, CO_RE skips adjust load's mem size and retains the
8-byte load.
The two fixes will be added in the next version.
Please review the solution to be selected.
Thanks,
Yang
