On 12/6/22 10:38 AM, Björn Töpel wrote:
Yonghong Song <yhs@xxxxxxxx> writes:
On 12/6/22 9:47 AM, Yonghong Song wrote:
On 12/6/22 5:21 AM, Ilya Leoshkevich wrote:
On Fri, 2022-12-02 at 11:36 +0100, Björn Töpel wrote:
From: Björn Töpel <bjorn@xxxxxxxxxxxx>
A BPF call instruction can be, correctly, marked with zext_dst set to
true. An example of this can be found in the BPF selftests
progs/bpf_cubic.c:
...
extern __u32 tcp_reno_undo_cwnd(struct sock *sk) __ksym;
__u32 BPF_STRUCT_OPS(bpf_cubic_undo_cwnd, struct sock *sk)
{
return tcp_reno_undo_cwnd(sk);
}
...
which compiles to:
0: r1 = *(u64 *)(r1 + 0x0)
1: call -0x1
2: exit
The call will be marked as zext_dst set to true, and for some
backends
(bpf_jit_needs_zext() returns true) expanded to:
0: r1 = *(u64 *)(r1 + 0x0)
1: call -0x1
2: w0 = w0
3: exit
In the verifier, the marking is done by check_kfunc_call() (added in
e6ac2450d6de), right? So the problem occurs only for kfuncs?
/* Check return type */
t = btf_type_skip_modifiers(desc_btf, func_proto->type, NULL);
...
if (btf_type_is_scalar(t)) {
mark_reg_unknown(env, regs, BPF_REG_0);
mark_btf_func_reg_size(env, BPF_REG_0, t->size);
I tried to find some official information whether the eBPF calling
convention requires sign- or zero- extending return values and
arguments, but unfortunately [1] doesn't mention this.
LLVM's lib/Target/BPF/BPFCallingConv.td mentions both R* and W*
registers, but since assigning to W* leads to zero-extension, it seems
to me that this is the case.
We actually follow the clang convention, the zero-extension is either
done in caller or callee, but not both. See
https://reviews.llvm.org/D131598 how the convention could be changed.
The following is an example.
$ cat t.c
extern unsigned foo(void);
unsigned bar1(void) {
return foo();
}
unsigned bar2(void) {
if (foo()) return 10; else return 20;
}
$ clang -target bpf -mcpu=v3 -O2 -c t.c && llvm-objdump -d t.o
t.o: file format elf64-bpf
Disassembly of section .text:
0000000000000000 <bar1>:
0: 85 10 00 00 ff ff ff ff call -0x1
1: 95 00 00 00 00 00 00 00 exit
0000000000000010 <bar2>:
2: 85 10 00 00 ff ff ff ff call -0x1
3: bc 01 00 00 00 00 00 00 w1 = w0
4: b4 00 00 00 14 00 00 00 w0 = 0x14
5: 16 01 01 00 00 00 00 00 if w1 == 0x0 goto +0x1 <LBB1_2>
6: b4 00 00 00 0a 00 00 00 w0 = 0xa
0000000000000038 <LBB1_2>:
7: 95 00 00 00 00 00 00 00 exit
$
If the return value of 'foo()' is actually used in the bpf program, the
proper zero extension will be done. Otherwise, it is not done.
This is with latest llvm16. I guess we need to check llvm whether
we could enforce to add a w0 = w0 in bar1().
Otherwise, with this patch, it will add w0 = w0 in all cases which
is not necessary in most of practical cases.
If the above is correct, then shouldn't we rather use sizeof(void *) in
the mark_btf_func_reg_size() call above?
The opt_subreg_zext_lo32_rnd_hi32() function which is responsible for
the zext patching, relies on insn_def_regno() to fetch the register
to
zero-extend. However, this function does not handle call instructions
correctly, and opt_subreg_zext_lo32_rnd_hi32() fails the
verification.
Make sure that R0 is correctly resolved for (BPF_JMP | BPF_CALL)
instructions.
Fixes: 83a2881903f3 ("bpf: Account for BPF_FETCH in
insn_has_def32()")
Signed-off-by: Björn Töpel <bjorn@xxxxxxxxxxxx>
---
I'm not super happy about the additional special case -- first
cmpxchg, and now call. :-( A more elegant/generic solution is
welcome!
---
kernel/bpf/verifier.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 264b3dc714cc..4f9660eafc72 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -13386,6 +13386,9 @@ static int
opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
if (!bpf_jit_needs_zext() && !is_cmpxchg_insn(&insn))
continue;
+ if (insn.code == (BPF_JMP | BPF_CALL))
+ load_reg = BPF_REG_0;
Want to double check. Do we actually have a problem here?
For example, on x64, we probably won't have this issue.
The "problem" is that I hit this:
if (WARN_ON(load_reg == -1)) {
verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
return -EFAULT;
}
This path is only taken for archs which have bpf_jit_needs_zext() ==
true. In my case it's riscv64, but it should hit i386, sparc, s390, ppc,
mips, and arm.
My reading of this thread has been that "marking the call has
zext_dst=true, is incorrect", i.e. that LLVM will insert the correct
zext instructions.
Your interpretation is correct. Yes, for func return values, the
llvm will insert correct zext/sext instructions if the return
value is used. Otherwise, if the return value simply passes
through, the caller call site should handle that properly.
So, yes changing t->size to sizeof(u64) in below code in
check_kfunc_call() should work. But the fix sounds like a hack
and we might have some side effect during verification, now
or future.
Maybe we could check BPF_PSEUDO_KFUNC_CALL in appropriate place to
prevent zext.
So, on way of not hitting this path, is what Ilya suggest -- in
check_kfunc_call():
if (btf_type_is_scalar(t)) {
mark_reg_unknown(env, regs, BPF_REG_0);
mark_btf_func_reg_size(env, BPF_REG_0, t->size);
}
change t->size to sizeof(u64). Then the call wont be marked.
>>> ...
>>> extern __u32 tcp_reno_undo_cwnd(struct sock *sk) __ksym;
>>>
>>> __u32 BPF_STRUCT_OPS(bpf_cubic_undo_cwnd, struct sock *sk)
>>> {
>>> return tcp_reno_undo_cwnd(sk);
>>> }
The native code will return a 32-bit subreg to bpf program,
and bpf didn't do anything and return r0 to the kernel func.
In the kernel func, the kernel will take 32-bit subreg by
x86_64 convention. This applies to some other return types
like u8/s8/u16/s16/u32/s32.
Which architecture you actually see the issue?
This is riscv64, but the nature of the problem is more of an assertion
failure, than codegen AFAIK.
I hit is when I load progs/bpf_cubic.o from the selftest. Nightly clang
from apt.llvm.org: clang version 16.0.0
(++20221204034339+7a194cfb327a-1~exp1~20221204154444.167)
Björn
