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

[Udip Pant]

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

On Tue, Aug 25, 2020 at 4:21 PM Udip Pant <udippant@xxxxxx> 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@xxxxxx>
>> ---
>>  .../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).