Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200825232003.2877030-1-udippant@fb.com> <20200825232003.2877030-3-udippant@fb.com>
In-Reply-To: <20200825232003.2877030-3-udippant@fb.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Wed, 26 Aug 2020 23:04:49 -0700
Message-ID: <CAEf4BzYsxBJf2a59L4EPKwX0eH2U7z41PSUgupwOWUXVH4sgYQ@mail.gmail.com>
Subject: Re: [PATCH bpf-next v3 2/4] selftests/bpf: add test for freplace
 program with write access
To:     Udip Pant <udippant@fb.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        Andrii Nakryiko <andriin@fb.com>,
        "David S . Miller" <davem@davemloft.net>,
        Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Aug 25, 2020 at 4:21 PM Udip Pant <udippant@fb.com> wrote:
>
> This adds a selftest that tests the behavior when a freplace target program
> attempts to make a write access on a packet. The expectation is that the read or write
> access is granted based on the program type of the linked program and
> not itself (which is of type, for e.g., BPF_PROG_TYPE_EXT).
>
> This test fails without the associated patch on the verifier.
>
> Signed-off-by: Udip Pant <udippant@fb.com>
> ---
>  .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  |  1 +
>  .../selftests/bpf/progs/fexit_bpf2bpf.c       | 27 +++++++++++++++++++
>  .../selftests/bpf/progs/test_pkt_access.c     | 20 ++++++++++++++
>  3 files changed, 48 insertions(+)
>

[...]

> +__attribute__ ((noinline))
> +int test_pkt_write_access_subprog(struct __sk_buff *skb, __u32 off)
> +{
> +       void *data = (void *)(long)skb->data;
> +       void *data_end = (void *)(long)skb->data_end;
> +       struct tcphdr *tcp = NULL;
> +
> +       if (off > sizeof(struct ethhdr) + sizeof(struct ipv6hdr))
> +               return -1;
> +
> +       tcp = data + off;
> +       if (tcp + 1 > data_end)
> +               return -1;
> +       /* make modification to the packet data */
> +       tcp->check++;

Just FYI for all BPF contributors. This change makes test_pkt_access
BPF program to fail on kernel 5.5, which (the kernel) we use as part
libbpf CI testing. test_pkt_access.o in turn makes few different
selftests (see [0] for details) to fail on 5.5 (because
test_pkt_access is used as one of BPF objects loaded as part of those
selftests). This is ok, I'm blacklisting (at least temporarily) those
tests, but I wanted to bring up this issue, as it did happen before
and will keep happening in the future and will constantly decrease
test coverage for older kernels that libbpf CI performs.

I propose that when we introduce new features (like new fields in a
BPF program's context or something along those lines) and want to test
them, we should lean towards creating new tests, not modify existing
ones. This will allow all already working selftests to keep working
for older kernels. Does this sound reasonable as an approach?

As for this particular breakage, I'd appreciate someone taking a look
at the problem and checking if it's some new feature that's not
present in 5.5 or just Clang/verifier interactions (32-bit pointer
arithmetic, is this a new issue?). If it's something fixable, it would
be nice to fix and restore 5.5 tests. Thanks!

  [0] https://travis-ci.com/github/libbpf/libbpf/jobs/378226438

Verifier complains about:

; if (test_pkt_write_access_subprog(skb, (void *)tcp - data))

57: (79) r1 = *(u64 *)(r10 -8)

58: (bc) w2 = w1

59: (1c) w2 -= w9

R2 32-bit pointer arithmetic prohibited

processed 198 insns (limit 1000000) max_states_per_insn 1 total_states
8 peak_states 8 mark_read 7


> +       return 0;
> +}
> +
>  SEC("classifier/test_pkt_access")
>  int test_pkt_access(struct __sk_buff *skb)
>  {
> @@ -117,6 +135,8 @@ int test_pkt_access(struct __sk_buff *skb)
>         if (test_pkt_access_subprog3(3, skb) != skb->len * 3 * skb->ifindex)
>                 return TC_ACT_SHOT;
>         if (tcp) {
> +               if (test_pkt_write_access_subprog(skb, (void *)tcp - data))
> +                       return TC_ACT_SHOT;
>                 if (((void *)(tcp) + 20) > data_end || proto != 6)
>                         return TC_ACT_SHOT;
>                 barrier(); /* to force ordering of checks */
> --
> 2.24.1
>