On Tue, Aug 11, 2020 at 09:08:13AM -0700, Yonghong Song wrote:
>
>
> 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".
aaah, ok ;-)
>
> 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?
Jiri (Olsa)
olsajiri@xxxxxxxxx
thank,
jirka
