Re: [PATCH bpf-next v3 2/4] selftests/bpf: add test for freplace program with write access

From: Udip Pant <udippant@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>, Martin 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>
Subject: Re: [PATCH bpf-next v3 2/4] selftests/bpf: add test for freplace program with write access
Date: Thu, 27 Aug 2020 19:41:59 +0000	[thread overview]
Message-ID: <F03D5838-0E4D-4B55-B8CA-42FABEA09A05@fb.com> (raw)
In-Reply-To: <CAEf4BzYsxBJf2a59L4EPKwX0eH2U7z41PSUgupwOWUXVH4sgYQ@mail.gmail.com>

On 8/26/20, 11:05 PM, "Andrii Nakryiko" <andrii.nakryiko@gmail.com> wrote:

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://urldefense.proofpoint.com/v2/url?u=https-3A__travis-2Dci.com_github_libbpf_libbpf_jobs_378226438&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=EICQWIMWWJPbefL5Ad4oTQ&m=ezWtkpxlrj19nFAuBX5LswTLCVR3zCtVY7MBlIsm7bA&s=zzYFn7QWYsp3BDOxYP95S4yKr2kqndGw1w6zHoNaRdU&e= 
>
> Verifier complains about:
>
> ; if (test_pkt_write_access_subprog(skb, (void *)tcp - data))

Not sure about this specific issue with 32-bit pointer arithmetic. But I can try a workaround to fix this test by passing only the skb (and deriving the tcp-header inside the method).