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 <[email protected]>
---
.../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(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
index 197d0d217b56..7c7168963d52 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
@@ -123,6 +123,7 @@ static void test_func_replace(void)
"freplace/get_skb_len",
"freplace/get_skb_ifindex",
"freplace/get_constant",
+ "freplace/test_pkt_write_access_subprog",
};
test_fexit_bpf2bpf_common("./fexit_bpf2bpf.o",
"./test_pkt_access.o",
diff --git a/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c
index 98e1efe14549..49a84a3a2306 100644
--- a/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c
@@ -1,8 +1,10 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2019 Facebook */
#include <linux/stddef.h>
+#include <linux/if_ether.h>
#include <linux/ipv6.h>
#include <linux/bpf.h>
+#include <linux/tcp.h>
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_endian.h>
#include <bpf/bpf_tracing.h>
@@ -151,4 +153,29 @@ int new_get_constant(long val)
test_get_constant = 1;
return test_get_constant; /* original get_constant() returns val - 122 */
}
+
+__u64 test_pkt_write_access_subprog = 0;
+SEC("freplace/test_pkt_write_access_subprog")
+int new_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;
+
+ if (off > sizeof(struct ethhdr) + sizeof(struct ipv6hdr))
+ return -1;
+
+ tcp = data + off;
+ if (tcp + 1 > data_end)
+ return -1;
+
+ /* make modifications to the packet data */
+ tcp->check++;
+ tcp->syn = 0;
+
+ test_pkt_write_access_subprog = 1;
+ return 0;
+}
+
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_pkt_access.c b/tools/testing/selftests/bpf/progs/test_pkt_access.c
index e72eba4a93d2..852051064507 100644
--- a/tools/testing/selftests/bpf/progs/test_pkt_access.c
+++ b/tools/testing/selftests/bpf/progs/test_pkt_access.c
@@ -79,6 +79,24 @@ int get_skb_ifindex(int val, struct __sk_buff *skb, int var)
return skb->ifindex * val * var;
}
+__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++;
+ 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
On Tue, Aug 25, 2020 at 4:21 PM Udip Pant <[email protected]> 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 <[email protected]>
> ---
> .../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
>
On 8/26/20, 11:05 PM, "Andrii Nakryiko" <[email protected]> wrote:
On Tue, Aug 25, 2020 at 4:21 PM Udip Pant <[email protected]> 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 <[email protected]>
>> ---
>> .../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).