Hi All,
Andrey found that the following diff messes up the verifier logic:
diff --git a/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c
b/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c
index d2b38fa6a5b0..e83d0b48d80c 100644
--- a/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_sk_lookup_kern.c
@@ -73,6 +73,7 @@ int bpf_sk_lookup_test0(struct __sk_buff *skb)
tuple_len = ipv4 ? sizeof(tuple->ipv4) : sizeof(tuple->ipv6);
sk = bpf_sk_lookup_tcp(skb, tuple, tuple_len, BPF_F_CURRENT_NETNS, 0);
+ bpf_printk("sk=%d\n", sk ? 1 : 0);
if (sk)
bpf_sk_release(sk);
return sk ? TC_ACT_OK : TC_ACT_UNSPEC;
The generated llvm code is correct.
What is happening is that first "if(sk)" check converts "sock_or_null"
into "sock"
and second "if(sk)" no longer doing mark_ptr_or_null_regs() since
if (!is_jmp32 && BPF_SRC(insn->code) == BPF_K &&
insn->imm == 0 && (opcode == BPF_JEQ || opcode == BPF_JNE) &&
reg_type_may_be_null(dst_reg->type)) {
condition is no longer true.
The verifier has to follow both branches of second "if (sk)"
because is_branch_taken() doesn't prune paths with one reg being pointer.
Hence it reaches the end where the verifier thinks that sk wasn't released
and complains with:
43: (85) call bpf_sk_lookup_tcp#84
44: (bf) r6 = r0
45: (b7) r1 = 2660
46: (6b) *(u16 *)(r10 -4) = r1
47: (b7) r1 = 624782195
48: (63) *(u32 *)(r10 -8) = r1
49: (73) *(u8 *)(r10 -2) = r7
50: (b7) r3 = 1
51: (55) if r6 != 0x0 goto pc+1
from 51 to 53: R0=sock(id=0,ref_obj_id=3,off=0,imm=0) R1=inv624782195
R3=inv1 R6=sock(id=0,ref_obj_id=3,off=0,imm=0) R7=invP0 R10=fp0
fp-8=?0mmmmmm refs=3
53: (bf) r1 = r10
54: (07) r1 += -8
55: (b7) r2 = 7
56: (85) call bpf_trace_printk#6
57: (15) if r6 == 0x0 goto pc+2
from 57 to 60: R0_w=inv(id=0) R6=sock(id=0,ref_obj_id=3,off=0,imm=0)
R7=invP0 R10=fp0 fp-8=?mmmmmmm refs=3
60: (b7) r0 = -1
61: (15) if r6 == 0x0 goto pc+1
R0_w=inv-1 R6=sock(id=0,ref_obj_id=3,off=0,imm=0) R7=invP0 R10=fp0
fp-8=?mmmmmmm refs=3
62: (b7) r0 = 0
63: (95) exit
Unreleased reference id=3 alloc_insn=43
Insn 57 is where the bug is.
The verifier needs to see that this is impossible path.
"from 57 to 60:" with R6=sock should have never happened.
I think the best way to fix this is to teach is_branch_taken() to recognize
== and != comparison of pointer with zero.
wdyt?
Anyone can craft a fix?
