| Message ID | 20191211175349.245622-1-sdf@google.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next,1/2] bpf: expose __sk_buff wire_len/gso_segs to BPF_PROG_TEST_RUN | expand |
On Wed, Dec 11, 2019 at 09:53:48AM -0800, Stanislav Fomichev wrote: > wire_len should not be less than real len and is capped by GSO_MAX_SIZE. > gso_segs is capped by GSO_MAX_SEGS. Acked-by: Martin KaFai Lau <kafai@fb.com>
On Wed, Dec 11, 2019 at 9:53 AM Stanislav Fomichev <sdf@google.com> wrote: > > wire_len should not be less than real len and is capped by GSO_MAX_SIZE. > gso_segs is capped by GSO_MAX_SEGS. > > Signed-off-by: Stanislav Fomichev <sdf@google.com> This change breaks tests: ./test_progs -n 16 test_kfree_skb:PASS:prog_load sched cls 0 nsec test_kfree_skb:PASS:prog_load raw tp 0 nsec test_kfree_skb:PASS:find_prog 0 nsec test_kfree_skb:PASS:find_prog 0 nsec test_kfree_skb:PASS:find_prog 0 nsec test_kfree_skb:PASS:find global data 0 nsec test_kfree_skb:PASS:attach_raw_tp 0 nsec test_kfree_skb:PASS:attach fentry 0 nsec test_kfree_skb:PASS:attach fexit 0 nsec test_kfree_skb:PASS:find_perf_buf_map 0 nsec test_kfree_skb:PASS:perf_buf__new 0 nsec test_kfree_skb:FAIL:ipv6 err -1 errno 22 retval 0 duration 0 on_sample:PASS:check_size 0 nsec on_sample:PASS:check_meta_ifindex 0 nsec on_sample:PASS:check_cb8_0 0 nsec on_sample:PASS:check_cb32_0 0 nsec on_sample:PASS:check_eth 0 nsec on_sample:PASS:check_ip 0 nsec on_sample:PASS:check_tcp 0 nsec test_kfree_skb:PASS:perf_buffer__poll 0 nsec test_kfree_skb:PASS:get_result 0 nsec #16 kfree_skb:FAIL Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
On 12/13, Alexei Starovoitov wrote: > On Wed, Dec 11, 2019 at 9:53 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > wire_len should not be less than real len and is capped by GSO_MAX_SIZE. > > gso_segs is capped by GSO_MAX_SEGS. > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > This change breaks tests: > ./test_progs -n 16 > test_kfree_skb:PASS:prog_load sched cls 0 nsec > test_kfree_skb:PASS:prog_load raw tp 0 nsec > test_kfree_skb:PASS:find_prog 0 nsec > test_kfree_skb:PASS:find_prog 0 nsec > test_kfree_skb:PASS:find_prog 0 nsec > test_kfree_skb:PASS:find global data 0 nsec > test_kfree_skb:PASS:attach_raw_tp 0 nsec > test_kfree_skb:PASS:attach fentry 0 nsec > test_kfree_skb:PASS:attach fexit 0 nsec > test_kfree_skb:PASS:find_perf_buf_map 0 nsec > test_kfree_skb:PASS:perf_buf__new 0 nsec > test_kfree_skb:FAIL:ipv6 err -1 errno 22 retval 0 duration 0 > on_sample:PASS:check_size 0 nsec > on_sample:PASS:check_meta_ifindex 0 nsec > on_sample:PASS:check_cb8_0 0 nsec > on_sample:PASS:check_cb32_0 0 nsec > on_sample:PASS:check_eth 0 nsec > on_sample:PASS:check_ip 0 nsec > on_sample:PASS:check_tcp 0 nsec > test_kfree_skb:PASS:perf_buffer__poll 0 nsec > test_kfree_skb:PASS:get_result 0 nsec > #16 kfree_skb:FAIL > Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED Ugh, it's probably because of '__skb->wire_len < skb->len' check. Let me take a look. (sorry, I'm still not running/looking at full test_progs because BTF support is WIP in our toolchain and some subtests fail because of that, generating a bunch of noise).
On Fri, Dec 13, 2019 at 1:23 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > On 12/13, Alexei Starovoitov wrote: > > On Wed, Dec 11, 2019 at 9:53 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > wire_len should not be less than real len and is capped by GSO_MAX_SIZE. > > > gso_segs is capped by GSO_MAX_SEGS. > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > > This change breaks tests: > > ./test_progs -n 16 > > test_kfree_skb:PASS:prog_load sched cls 0 nsec > > test_kfree_skb:PASS:prog_load raw tp 0 nsec > > test_kfree_skb:PASS:find_prog 0 nsec > > test_kfree_skb:PASS:find_prog 0 nsec > > test_kfree_skb:PASS:find_prog 0 nsec > > test_kfree_skb:PASS:find global data 0 nsec > > test_kfree_skb:PASS:attach_raw_tp 0 nsec > > test_kfree_skb:PASS:attach fentry 0 nsec > > test_kfree_skb:PASS:attach fexit 0 nsec > > test_kfree_skb:PASS:find_perf_buf_map 0 nsec > > test_kfree_skb:PASS:perf_buf__new 0 nsec > > test_kfree_skb:FAIL:ipv6 err -1 errno 22 retval 0 duration 0 > > on_sample:PASS:check_size 0 nsec > > on_sample:PASS:check_meta_ifindex 0 nsec > > on_sample:PASS:check_cb8_0 0 nsec > > on_sample:PASS:check_cb32_0 0 nsec > > on_sample:PASS:check_eth 0 nsec > > on_sample:PASS:check_ip 0 nsec > > on_sample:PASS:check_tcp 0 nsec > > test_kfree_skb:PASS:perf_buffer__poll 0 nsec > > test_kfree_skb:PASS:get_result 0 nsec > > #16 kfree_skb:FAIL > > Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED > Ugh, it's probably because of '__skb->wire_len < skb->len' check. > Let me take a look. > > (sorry, I'm still not running/looking at full test_progs because BTF support > is WIP in our toolchain and some subtests fail because of that, > generating a bunch of noise). I thought all bpf-next developers are developing against that tree ? Are you saying you cannot install the latest clang/pahole on your development system? git pull llvm;ninja;ninja install; git pull pahole; cmake;make why is it not possible? Now your complains about skeleton make more sense, but it's an issue with your particular setup. Anyway I'm not sure that this test issue is actually an issue with your patch. It could be that this test is flaky in a weird way. Just with and without your kernel patch it's 100% reproducible for me and I need to keep the rest of the patches moving without introducing failures in my test setup. All that will get resolved when we have kernel CI.
On 12/13, Alexei Starovoitov wrote: > On Fri, Dec 13, 2019 at 1:23 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > On 12/13, Alexei Starovoitov wrote: > > > On Wed, Dec 11, 2019 at 9:53 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > > > wire_len should not be less than real len and is capped by GSO_MAX_SIZE. > > > > gso_segs is capped by GSO_MAX_SEGS. > > > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > > > > This change breaks tests: > > > ./test_progs -n 16 > > > test_kfree_skb:PASS:prog_load sched cls 0 nsec > > > test_kfree_skb:PASS:prog_load raw tp 0 nsec > > > test_kfree_skb:PASS:find_prog 0 nsec > > > test_kfree_skb:PASS:find_prog 0 nsec > > > test_kfree_skb:PASS:find_prog 0 nsec > > > test_kfree_skb:PASS:find global data 0 nsec > > > test_kfree_skb:PASS:attach_raw_tp 0 nsec > > > test_kfree_skb:PASS:attach fentry 0 nsec > > > test_kfree_skb:PASS:attach fexit 0 nsec > > > test_kfree_skb:PASS:find_perf_buf_map 0 nsec > > > test_kfree_skb:PASS:perf_buf__new 0 nsec > > > test_kfree_skb:FAIL:ipv6 err -1 errno 22 retval 0 duration 0 > > > on_sample:PASS:check_size 0 nsec > > > on_sample:PASS:check_meta_ifindex 0 nsec > > > on_sample:PASS:check_cb8_0 0 nsec > > > on_sample:PASS:check_cb32_0 0 nsec > > > on_sample:PASS:check_eth 0 nsec > > > on_sample:PASS:check_ip 0 nsec > > > on_sample:PASS:check_tcp 0 nsec > > > test_kfree_skb:PASS:perf_buffer__poll 0 nsec > > > test_kfree_skb:PASS:get_result 0 nsec > > > #16 kfree_skb:FAIL > > > Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED > > Ugh, it's probably because of '__skb->wire_len < skb->len' check. > > Let me take a look. > > > > (sorry, I'm still not running/looking at full test_progs because BTF support > > is WIP in our toolchain and some subtests fail because of that, > > generating a bunch of noise). > > I thought all bpf-next developers are developing against that tree ? I am developing against that tree, but I have a wrapper around make that points it to the proper version of cc/tools that we use for prod builds (for consistency). > Are you saying you cannot install the latest clang/pahole on your > development system? > git pull llvm;ninja;ninja install; > git pull pahole; cmake;make > why is it not possible? Yeah, I'll do that now, it just requires some manual movements :-) > Now your complains about skeleton make more sense, > but it's an issue with your particular setup. > > Anyway I'm not sure that this test issue is actually an issue with your patch. > It could be that this test is flaky in a weird way. Just with and without your > kernel patch it's 100% reproducible for me and I need to keep the rest > of the patches > moving without introducing failures in my test setup. > All that will get resolved when we have kernel CI. No, I'm pretty sure that's that wire_len check. I think I need to add a special case to set it to skb->len if it's zero. Will follow up with a v2!
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index 85c8cbbada92..06cadba2e3b9 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -263,8 +263,10 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb) return -EINVAL; /* tstamp is allowed */ + /* wire_len is allowed */ + /* gso_segs is allowed */ - if (!range_is_zero(__skb, offsetofend(struct __sk_buff, tstamp), + if (!range_is_zero(__skb, offsetofend(struct __sk_buff, gso_segs), sizeof(struct __sk_buff))) return -EINVAL; @@ -272,6 +274,14 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb) skb->tstamp = __skb->tstamp; memcpy(&cb->data, __skb->cb, QDISC_CB_PRIV_LEN); + if (__skb->wire_len < skb->len || __skb->wire_len > GSO_MAX_SIZE) + return -EINVAL; + cb->pkt_len = __skb->wire_len; + + if (__skb->gso_segs > GSO_MAX_SEGS) + return -EINVAL; + skb_shinfo(skb)->gso_segs = __skb->gso_segs; + return 0; } @@ -285,6 +295,8 @@ static void convert_skb_to___skb(struct sk_buff *skb, struct __sk_buff *__skb) __skb->priority = skb->priority; __skb->tstamp = skb->tstamp; memcpy(__skb->cb, &cb->data, QDISC_CB_PRIV_LEN); + __skb->wire_len = cb->pkt_len; + __skb->gso_segs = skb_shinfo(skb)->gso_segs; } int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
wire_len should not be less than real len and is capped by GSO_MAX_SIZE. gso_segs is capped by GSO_MAX_SEGS. Signed-off-by: Stanislav Fomichev <sdf@google.com> --- net/bpf/test_run.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)