Message-ID: <20200810135451.GA699846@krava>
Date:   Mon, 10 Aug 2020 15:54:51 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Yonghong Song <yhs@...com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, Andrii Nakryiko <andriin@...com>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>
Subject: Re: [RFC] bpf: verifier check for dead branch

On Sun, Aug 09, 2020 at 06:21:01PM -0700, Yonghong Song wrote:
> 
> 
> On 8/7/20 10:30 AM, Jiri Olsa wrote:
> > hi,
> > we have a customer facing some odd verifier fails on following
> > sk_skb program:
> > 
> >     0. r2 = *(u32 *)(r1 + data_end)
> >     1. r4 = *(u32 *)(r1 + data)
> >     2. r3 = r4
> >     3. r3 += 42
> >     4. r1 = 0
> >     5. if r3 > r2 goto 8
> >     6. r4 += 14
> >     7. r1 = r4
> >     8. if r3 > r2 goto 10
> >     9. r2 = *(u8 *)(r1 + 9)
> >    10. r0 = 0
> >    11. exit
> > 
> > The code checks if the skb data is big enough (5) and if it is,
> > it prepares pointer in r1 (7), then there's again size check (8)
> > and finally data load from r1 (9).
> > 
> > It's and odd code, but apparently this is something that can
> > get produced by clang.
> 
> Could you provide a test case where clang generates the above code?
> I would like to see whether clang can do a better job to avoid
> such codes.

I get that code genrated by using recent enough upstream clang
on the attached source.

	/opt/clang/bin/clang --version
	clang version 11.0.0 (https://github.com/llvm/llvm-project.git 4cbfb98eb362b0629d5d1cd113af4427e2904763)
	Target: x86_64-unknown-linux-gnu
	Thread model: posix
	InstalledDir: /opt/clang/bin

	$ llvm-objdump -d verifier-cond-repro.o 

	verifier-cond-repro.o:  file format ELF64-BPF

	Disassembly of section .text:

	0000000000000000 my_prog:
	       0:       61 12 50 00 00 00 00 00 r2 = *(u32 *)(r1 + 80)
	       1:       61 14 4c 00 00 00 00 00 r4 = *(u32 *)(r1 + 76)
	       2:       bf 43 00 00 00 00 00 00 r3 = r4
	       3:       07 03 00 00 2a 00 00 00 r3 += 42
	       4:       b7 01 00 00 00 00 00 00 r1 = 0
	       5:       2d 23 02 00 00 00 00 00 if r3 > r2 goto +2 <LBB0_2>
	       6:       07 04 00 00 0e 00 00 00 r4 += 14
	       7:       bf 41 00 00 00 00 00 00 r1 = r4

	0000000000000040 LBB0_2:
	       8:       2d 23 05 00 00 00 00 00 if r3 > r2 goto +5 <LBB0_5>
	       9:       71 12 09 00 00 00 00 00 r2 = *(u8 *)(r1 + 9)
	      10:       56 02 03 00 11 00 00 00 if w2 != 17 goto +3 <LBB0_5>
	      11:       b4 00 00 00 d2 04 00 00 w0 = 1234
	      12:       69 11 16 00 00 00 00 00 r1 = *(u16 *)(r1 + 22)
	      13:       16 01 01 00 d2 04 00 00 if w1 == 1234 goto +1 <LBB0_6>

	0000000000000070 LBB0_5:
	      14:       b4 00 00 00 ff ff ff ff w0 = -1

	0000000000000078 LBB0_6:
	      15:       95 00 00 00 00 00 00 00 exit


thanks,
jirka


---
// Copyright (c) 2019 Tigera, Inc. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include <stddef.h>
#include <string.h>
#include <linux/bpf.h>
#include <linux/if_ether.h>
#include <linux/if_packet.h>
#include <linux/ip.h>
#include <linux/ipv6.h>
#include <linux/in.h>
#include <linux/udp.h>
#include <linux/tcp.h>
#include <linux/pkt_cls.h>
#include <sys/socket.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_endian.h>

#include <stddef.h>

#define INLINE inline __attribute__((always_inline))

#define skb_shorter(skb, len) ((void *)(long)(skb)->data + (len) > (void *)(long)skb->data_end)

#define ETH_IPV4_UDP_SIZE (14+20+8)

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;
}

int my_prog(struct __sk_buff *skb)
{
	struct iphdr *ip = NULL;
	struct udphdr *udp;
	__u8 proto = 0;

	if (!(ip = get_iphdr(skb)))
		goto out;

	proto = ip->protocol;

	if (proto != IPPROTO_UDP)
		goto out;

	udp = (void*)(ip + 1);

	if (udp->dest != 1234)
		goto out;

	if (!udp)
		goto out;

	return udp->dest;

out:
	return -1;
}

Powered by blists - more mailing lists