Alexei Starovoitov wrote:
> 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?
Seems reasonable to me. I've also hit this recently fwiw.
> Anyone can craft a fix?
If no one beats me to it I can probably craft something tomorrow.
