From: Yonghong Song <yhs@fb.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>, <bpf@vger.kernel.org>,
<netdev@vger.kernel.org>, Andrii Nakryiko <andriin@fb.com>,
Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@chromium.org>
Subject: Re: [RFC] bpf: verifier check for dead branch
Date: Tue, 11 Aug 2020 09:08:13 -0700 [thread overview]
Message-ID: <f03e2ce3-8cf8-0590-1777-f9e8171cd3fa@fb.com> (raw)
In-Reply-To: <20200811071438.GC699846@krava>
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
>
next prev parent reply other threads:[~2020-08-11 16:09 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-07 17:30 [RFC] bpf: verifier check for dead branch Jiri Olsa
2020-08-10 1:21 ` Yonghong Song
2020-08-10 13:54 ` Jiri Olsa
2020-08-10 17:16 ` Yonghong Song
2020-08-11 7:14 ` Jiri Olsa
2020-08-11 16:08 ` Yonghong Song [this message]
2020-08-12 7:48 ` Jiri Olsa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f03e2ce3-8cf8-0590-1777-f9e8171cd3fa@fb.com \
--to=yhs@fb.com \
--cc=andriin@fb.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.com \
--cc=jolsa@redhat.com \
--cc=kafai@fb.com \
--cc=kpsingh@chromium.org \
--cc=netdev@vger.kernel.org \
--cc=songliubraving@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).