This bit of refactoring aims to simplify the next patch in this series, in which freeing 'data' is a bit less straightforward. Signed-off-by: Paul Chaignon --- net/bpf/test_run.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index 4a862d605386..4e595b7ad94f 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -1009,8 +1009,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff)); if (IS_ERR(ctx)) { - kfree(data); - return PTR_ERR(ctx); + ret = PTR_ERR(ctx); + goto out; } switch (prog->type) { @@ -1030,24 +1030,23 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, sk = sk_alloc(net, AF_UNSPEC, GFP_USER, &bpf_dummy_proto, 1); if (!sk) { - kfree(data); - kfree(ctx); - return -ENOMEM; + ret = -ENOMEM; + goto out; } sock_init_data(NULL, sk); skb = slab_build_skb(data); if (!skb) { - kfree(data); - kfree(ctx); - sk_free(sk); - return -ENOMEM; + ret = -ENOMEM; + goto out; } skb->sk = sk; skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN); __skb_put(skb, size); + data = NULL; /* data released via kfree_skb */ + if (ctx && ctx->ifindex > 1) { dev = dev_get_by_index(net, ctx->ifindex); if (!dev) { @@ -1139,7 +1138,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, if (dev && dev != net->loopback_dev) dev_put(dev); kfree_skb(skb); - sk_free(sk); + kfree(data); + if (sk) + sk_free(sk); kfree(ctx); return ret; } -- 2.43.0 This patch adds support for crafting non-linear skbs in BPF test runs for tc programs, via a new flag BPF_F_TEST_SKB_NON_LINEAR. When this flag is set, only the L2 header is pulled in the linear area. This is particularly useful to test support for non-linear skbs in large codebases such as Cilium. We've had multiple bugs in the past few years where we were missing calls to bpf_skb_pull_data(). This support in BPF_PROG_TEST_RUN would allow us to automatically cover this case in our BPF tests. In addition to the selftests introduced later in the series, this patch was tested by setting BPF_F_TEST_SKB_NON_LINEAR for all tc selftests programs and checking test failures were expected. Suggested-by: Daniel Borkmann Signed-off-by: Paul Chaignon --- include/uapi/linux/bpf.h | 2 + net/bpf/test_run.c | 88 ++++++++++++++++++++++++---------- tools/include/uapi/linux/bpf.h | 2 + 3 files changed, 66 insertions(+), 26 deletions(-) diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 233de8677382..d77c8bf4b131 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -1448,6 +1448,8 @@ enum { #define BPF_F_TEST_XDP_LIVE_FRAMES (1U << 1) /* If set, apply CHECKSUM_COMPLETE to skb and validate the checksum */ #define BPF_F_TEST_SKB_CHECKSUM_COMPLETE (1U << 2) +/* If set, skb will be non-linear */ +#define BPF_F_TEST_SKB_NON_LINEAR (1U << 3) /* type for BPF_ENABLE_STATS */ enum bpf_stats_type { diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index 4e595b7ad94f..645b7b5af08f 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -660,20 +660,29 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_memb_release, KF_RELEASE) BTF_KFUNCS_END(test_sk_check_kfunc_ids) static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size, - u32 size, u32 headroom, u32 tailroom) + u32 size, u32 headroom, u32 tailroom, bool nonlinear) { void __user *data_in = u64_to_user_ptr(kattr->test.data_in); - void *data; + void *data, *dst; if (user_size < ETH_HLEN || user_size > PAGE_SIZE - headroom - tailroom) return ERR_PTR(-EINVAL); - size = SKB_DATA_ALIGN(size); - data = kzalloc(size + headroom + tailroom, GFP_USER); + /* In non-linear case, data_in is copied to the paged data */ + if (nonlinear) { + data = alloc_page(GFP_USER); + } else { + size = SKB_DATA_ALIGN(size); + data = kzalloc(size + headroom + tailroom, GFP_USER); + } if (!data) return ERR_PTR(-ENOMEM); - if (copy_from_user(data + headroom, data_in, user_size)) { + if (nonlinear) + dst = page_address(data); + else + dst = data + headroom; + if (copy_from_user(dst, data_in, user_size)) { kfree(data); return ERR_PTR(-EFAULT); } @@ -984,7 +993,7 @@ static struct proto bpf_dummy_proto = { int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, union bpf_attr __user *uattr) { - bool is_l2 = false, is_direct_pkt_access = false; + bool is_l2 = false, is_direct_pkt_access = false, is_nonlinear = false; struct net *net = current->nsproxy->net_ns; struct net_device *dev = net->loopback_dev; u32 size = kattr->test.data_size_in; @@ -997,21 +1006,11 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, void *data; int ret; - if ((kattr->test.flags & ~BPF_F_TEST_SKB_CHECKSUM_COMPLETE) || + if ((kattr->test.flags & ~(BPF_F_TEST_SKB_CHECKSUM_COMPLETE | BPF_F_TEST_SKB_NON_LINEAR)) || kattr->test.cpu || kattr->test.batch_size) return -EINVAL; - data = bpf_test_init(kattr, kattr->test.data_size_in, - size, NET_SKB_PAD + NET_IP_ALIGN, - SKB_DATA_ALIGN(sizeof(struct skb_shared_info))); - if (IS_ERR(data)) - return PTR_ERR(data); - - ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff)); - if (IS_ERR(ctx)) { - ret = PTR_ERR(ctx); - goto out; - } + is_nonlinear = kattr->test.flags & BPF_F_TEST_SKB_NON_LINEAR; switch (prog->type) { case BPF_PROG_TYPE_SCHED_CLS: @@ -1028,6 +1027,22 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, break; } + if (is_nonlinear && !is_l2) + return -EINVAL; + + data = bpf_test_init(kattr, kattr->test.data_size_in, + size, NET_SKB_PAD + NET_IP_ALIGN, + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), + is_nonlinear); + if (IS_ERR(data)) + return PTR_ERR(data); + + ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff)); + if (IS_ERR(ctx)) { + ret = PTR_ERR(ctx); + goto out; + } + sk = sk_alloc(net, AF_UNSPEC, GFP_USER, &bpf_dummy_proto, 1); if (!sk) { ret = -ENOMEM; @@ -1035,15 +1050,32 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, } sock_init_data(NULL, sk); - skb = slab_build_skb(data); + if (is_nonlinear) + skb = alloc_skb(NET_SKB_PAD + NET_IP_ALIGN + size + + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), + GFP_USER); + else + skb = slab_build_skb(data); if (!skb) { ret = -ENOMEM; goto out; } + skb->sk = sk; skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN); - __skb_put(skb, size); + + if (is_nonlinear) { + skb_fill_page_desc(skb, 0, data, 0, size); + skb->truesize += PAGE_SIZE; + skb->data_len = size; + skb->len = size; + + /* eth_type_trans expects the Ethernet header in the linear area. */ + __pskb_pull_tail(skb, ETH_HLEN); + } else { + __skb_put(skb, size); + } data = NULL; /* data released via kfree_skb */ @@ -1126,9 +1158,11 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, convert_skb_to___skb(skb, ctx); size = skb->len; - /* bpf program can never convert linear skb to non-linear */ - if (WARN_ON_ONCE(skb_is_nonlinear(skb))) + if (skb_is_nonlinear(skb)) { + /* bpf program can never convert linear skb to non-linear */ + WARN_ON_ONCE(!is_nonlinear); size = skb_headlen(skb); + } ret = bpf_test_finish(kattr, uattr, skb->data, NULL, size, retval, duration); if (!ret) @@ -1138,7 +1172,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, if (dev && dev != net->loopback_dev) dev_put(dev); kfree_skb(skb); - kfree(data); + if (data) + is_nonlinear ? __free_page(data) : kfree(data); if (sk) sk_free(sk); kfree(ctx); @@ -1264,7 +1299,7 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, size = max_data_sz; } - data = bpf_test_init(kattr, size, max_data_sz, headroom, tailroom); + data = bpf_test_init(kattr, size, max_data_sz, headroom, tailroom, false); if (IS_ERR(data)) { ret = PTR_ERR(data); goto free_ctx; @@ -1387,7 +1422,7 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog, if (size < ETH_HLEN) return -EINVAL; - data = bpf_test_init(kattr, kattr->test.data_size_in, size, 0, 0); + data = bpf_test_init(kattr, kattr->test.data_size_in, size, 0, 0, false); if (IS_ERR(data)) return PTR_ERR(data); @@ -1660,7 +1695,8 @@ int bpf_prog_test_run_nf(struct bpf_prog *prog, data = bpf_test_init(kattr, kattr->test.data_size_in, size, NET_SKB_PAD + NET_IP_ALIGN, - SKB_DATA_ALIGN(sizeof(struct skb_shared_info))); + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), + false); if (IS_ERR(data)) return PTR_ERR(data); diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 233de8677382..d77c8bf4b131 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -1448,6 +1448,8 @@ enum { #define BPF_F_TEST_XDP_LIVE_FRAMES (1U << 1) /* If set, apply CHECKSUM_COMPLETE to skb and validate the checksum */ #define BPF_F_TEST_SKB_CHECKSUM_COMPLETE (1U << 2) +/* If set, skb will be non-linear */ +#define BPF_F_TEST_SKB_NON_LINEAR (1U << 3) /* type for BPF_ENABLE_STATS */ enum bpf_stats_type { -- 2.43.0 Contrary to most flags currently used in selftests, the BPF_F_TEST_SKB_NON_LINEAR flag is not passed at program loading time, but when calling BPF_PROG_TEST_RUN. This patch updates the test loader to support it. Signed-off-by: Paul Chaignon --- tools/testing/selftests/bpf/test_loader.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c index a9388ac88358..4e0a03276f6b 100644 --- a/tools/testing/selftests/bpf/test_loader.c +++ b/tools/testing/selftests/bpf/test_loader.c @@ -91,6 +91,7 @@ struct test_spec { const char *btf_custom_path; int log_level; int prog_flags; + int run_flags; int mode_mask; int arch_mask; int load_mask; @@ -554,6 +555,8 @@ static int parse_test_spec(struct test_loader *tester, update_flags(&spec->prog_flags, BPF_F_XDP_HAS_FRAGS, clear); } else if (strcmp(val, "BPF_F_TEST_REG_INVARIANTS") == 0) { update_flags(&spec->prog_flags, BPF_F_TEST_REG_INVARIANTS, clear); + } else if (strcmp(val, "BPF_F_TEST_SKB_NON_LINEAR") == 0) { + update_flags(&spec->run_flags, BPF_F_TEST_SKB_NON_LINEAR, clear); } else /* assume numeric value */ { err = parse_int(val, &flags, "test prog flags"); if (err) @@ -854,7 +857,7 @@ static bool is_unpriv_capable_map(struct bpf_map *map) } } -static int do_prog_test_run(int fd_prog, int *retval, bool empty_opts) +static int do_prog_test_run(int fd_prog, int *retval, bool empty_opts, int run_flags) { __u8 tmp_out[TEST_DATA_LEN << 2] = {}; __u8 tmp_in[TEST_DATA_LEN] = {}; @@ -864,6 +867,7 @@ static int do_prog_test_run(int fd_prog, int *retval, bool empty_opts) .data_size_in = sizeof(tmp_in), .data_out = tmp_out, .data_size_out = sizeof(tmp_out), + .flags = run_flags, .repeat = 1, ); @@ -1103,7 +1107,8 @@ void run_subtest(struct test_loader *tester, } err = do_prog_test_run(bpf_program__fd(tprog), &retval, - bpf_program__type(tprog) == BPF_PROG_TYPE_SYSCALL ? true : false); + bpf_program__type(tprog) == BPF_PROG_TYPE_SYSCALL ? true : false, + spec->run_flags); if (!err && retval != subspec->retval && subspec->retval != POINTER_VALUE) { PRINT_FAIL("Unexpected retval: %d != %d\n", retval, subspec->retval); goto tobj_cleanup; -- 2.43.0 This patch adds two new selftests in the direct packet access suite, to cover the non-linear case with BPF_F_TEST_SKB_NON_LINEAR. The first tests the behavior of the bounds check with a non-linear skb. The second extends the first with a call to bpf_skb_pull_data() to be able to access the packet. Signed-off-by: Paul Chaignon --- .../bpf/progs/verifier_direct_packet_access.c | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tools/testing/selftests/bpf/progs/verifier_direct_packet_access.c b/tools/testing/selftests/bpf/progs/verifier_direct_packet_access.c index 28b602ac9cbe..fe06d7323f2f 100644 --- a/tools/testing/selftests/bpf/progs/verifier_direct_packet_access.c +++ b/tools/testing/selftests/bpf/progs/verifier_direct_packet_access.c @@ -800,4 +800,52 @@ l0_%=: /* exit(0) */ \ : __clobber_all); } +SEC("tc") +__description("direct packet access: test31 (non-linear, bad access)") +__success __retval(1) +__flag(BPF_F_TEST_SKB_NON_LINEAR) +__naked void access_test31_non_linear_bad_access(void) +{ + asm volatile (" \ + r2 = *(u32*)(r1 + %[__sk_buff_data]); \ + r3 = *(u32*)(r1 + %[__sk_buff_data_end]); \ + r0 = r2; \ + r0 += 22; \ + if r0 > r3 goto l0_%=; \ + r0 = *(u8*)(r0 - 1); \ + exit; \ +l0_%=: r0 = 1; \ + exit; \ +" : + : __imm_const(__sk_buff_data, offsetof(struct __sk_buff, data)), + __imm_const(__sk_buff_data_end, offsetof(struct __sk_buff, data_end)) + : __clobber_all); +} + +SEC("tc") +__description("direct packet access: test32 (non-linear, good access)") +__success __retval(0) +__flag(BPF_F_TEST_SKB_NON_LINEAR) +__naked void access_test32_non_linear_good_access(void) +{ + asm volatile (" \ + r6 = r1; \ + r2 = 22; \ + call %[bpf_skb_pull_data]; \ + r2 = *(u32*)(r6 + %[__sk_buff_data]); \ + r3 = *(u32*)(r6 + %[__sk_buff_data_end]); \ + r0 = r2; \ + r0 += 22; \ + if r0 > r3 goto l0_%=; \ + r0 = *(u8*)(r0 - 1); \ + exit; \ +l0_%=: r0 = 1; \ + exit; \ +" : + : __imm(bpf_skb_pull_data), + __imm_const(__sk_buff_data, offsetof(struct __sk_buff, data)), + __imm_const(__sk_buff_data_end, offsetof(struct __sk_buff, data_end)) + : __clobber_all); +} + char _license[] SEC("license") = "GPL"; -- 2.43.0