| Message ID | 20200825232003.2877030-3-udippant@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: verifier: use target program's type for access verifications | expand |
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 >
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).
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 */
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(+)