Re: [RFC] bpf: verifier check for dead branch

[Yonghong Song]

On 8/11/20 12:14 AM, Jiri Olsa wrote:

On Mon, Aug 10, 2020 at 10:16:12AM -0700, Yonghong Song wrote:

SNIP

Thanks for the test case. I can reproduce the issue. The following
is why this happens in llvm.
the pseudo IR code looks like
    data = skb->data
    data_end = skb->data_end
    comp = data + 42 > data_end
    ip = select "comp" nullptr "data + some offset"
          <=== select return one of nullptr or "data + some offset" based on
"comp"
    if comp   // original skb_shorter condition
       ....
    ...
       = ip

In llvm, bpf backend "select" actually inlined "comp" to generate proper
control flow. Therefore, comp is computed twice like below
    data = skb->data
    data_end = skb->data_end
    if (data + 42 > data_end) {
       ip = nullptr; goto block1;
    } else {
       ip = data + some_offset;
       goto block2;
    }
    ...
    if (data + 42 > data_end) // original skb_shorter condition

The issue can be workarounded the source. Just check data + 42 > data_end
and if failure return. Then you will be able to assign
a value to "ip" conditionally.

sorry for typo. The above should be "conditionally" -> "unconditionally".

is the change below what you mean? it produces the same code for me:

        diff --git a/tools/testing/selftests/bpf/progs/verifier-cond-repro.c 
b/tools/testing/selftests/bpf/progs/verifier-cond-repro.c
        index 2f11027d7e67..9c401bd00ab7 100644
        --- a/tools/testing/selftests/bpf/progs/verifier-cond-repro.c
        +++ b/tools/testing/selftests/bpf/progs/verifier-cond-repro.c
        @@ -41,12 +41,10 @@ static INLINE struct iphdr *get_iphdr (struct 
__sk_buff *skb)
                struct ethhdr *eth;
        
                if (skb_shorter(skb, ETH_IPV4_UDP_SIZE))
        -               goto out;
        +               return NULL;
        
                eth = (void *)(long)skb->data;
                ip = (void *)(eth + 1);
        -
        -out:
                return ip;
         }
        

I also tried this one:

        diff --git a/tools/testing/selftests/bpf/progs/verifier-cond-repro.c 
b/tools/testing/selftests/bpf/progs/verifier-cond-repro.c
        index 2f11027d7e67..00ff06fe6fdd 100644
        --- a/tools/testing/selftests/bpf/progs/verifier-cond-repro.c
        +++ b/tools/testing/selftests/bpf/progs/verifier-cond-repro.c
        @@ -57,7 +57,7 @@ int my_prog(struct __sk_buff *skb)
                __u8 proto = 0;
        
                if (!(ip = get_iphdr(skb)))
        -               goto out;
        +               return -1;
        
                proto = ip->protocol;

it did just slight change in generated code - added 'w0 = -1'
before the second condition

The following is what I mean:

diff --git a/t.c b/t.c
index c6baf28..7bf90dc 100644
--- a/t.c
+++ b/t.c
@@ -37,17 +37,10 @@

 static INLINE struct iphdr *get_iphdr (struct __sk_buff *skb)
 {
-       struct iphdr *ip = NULL;
        struct ethhdr *eth;

-       if (skb_shorter(skb, ETH_IPV4_UDP_SIZE))
-               goto out;
-
        eth = (void *)(long)skb->data;
-       ip = (void *)(eth + 1);
-
-out:
-       return ip;
+       return (void *)(eth + 1);
 }

 int my_prog(struct __sk_buff *skb)
@@ -56,9 +49,10 @@ int my_prog(struct __sk_buff *skb)
        struct udphdr *udp;
        __u8 proto = 0;

-       if (!(ip = get_iphdr(skb)))
+       if (skb_shorter(skb, ETH_IPV4_UDP_SIZE))
                goto out;

+       ip = get_iphdr(skb);
        proto = ip->protocol;

        if (proto != IPPROTO_UDP)

Will try to fix this issue in llvm12 as well.
Thanks!

great, could you please CC me on the changes?

This will be a llvm change. Do you have llvm phabricator login name
https://reviews.llvm.org/
so I can add you as a subscriber?

thanks a lot!
jirka