From: Jiri Olsa <jolsa@redhat.com>
To: Yonghong Song <yhs@fb.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:14:38 +0200 [thread overview]
Message-ID: <20200811071438.GC699846@krava> (raw)
In-Reply-To: <e4abe45b-2c80-9448-677c-e352f0ecb24e@fb.com>
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.
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
>
> Will try to fix this issue in llvm12 as well.
> Thanks!
great, could you please CC me on the changes?
thanks a lot!
jirka
next prev parent reply other threads:[~2020-08-11 7:14 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 [this message]
2020-08-11 16:08 ` Yonghong Song
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=20200811071438.GC699846@krava \
--to=jolsa@redhat.com \
--cc=andriin@fb.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@chromium.org \
--cc=netdev@vger.kernel.org \
--cc=songliubraving@fb.com \
--cc=yhs@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).