| Message ID | 20211012065705.224643-3-liujian56@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [PATHC,bpf,v5,1/3] skmsg: lose offset info in sk_psock_skb_ingress | expand |
Liu Jian wrote: > Add the test to check sockmap with strparser is working well. > > Signed-off-by: Liu Jian <liujian56@huawei.com> > --- > tools/testing/selftests/bpf/test_sockmap.c | 33 ++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 3 deletions(-) Hi Liu, This is a good test, but we should also add one with a parser returning a value that is not skb->len. This doesn't cover the case fixed in patch 1/3 correct? For that we would need to modify the BPF prog itself as well sockmap_parse_prog.c. For this patch though, Acked-by: John Fastabend <john.fastabend@gmail.com> Then one more patch is all we need something to break up the skb from the parser. We really need the test because its not something we can easily test otherwise and I don't have any use cases that do this so wouldn't catch it. Thanks! John
> -----Original Message----- > From: John Fastabend [mailto:john.fastabend@gmail.com] > Sent: Friday, October 22, 2021 11:31 PM > To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com; > daniel@iogearbox.net; jakub@cloudflare.com; lmb@cloudflare.com; > davem@davemloft.net; kuba@kernel.org; ast@kernel.org; > andrii@kernel.org; kafai@fb.com; songliubraving@fb.com; yhs@fb.com; > kpsingh@kernel.org; netdev@vger.kernel.org; bpf@vger.kernel.org; > xiyou.wangcong@gmail.com > Cc: liujian (CE) <liujian56@huawei.com> > Subject: RE: [PATHC bpf v5 3/3] selftests, bpf: Add one test for sockmap with > strparser > > Liu Jian wrote: > > Add the test to check sockmap with strparser is working well. > > > > Signed-off-by: Liu Jian <liujian56@huawei.com> > > --- > > tools/testing/selftests/bpf/test_sockmap.c | 33 > > ++++++++++++++++++++-- > > 1 file changed, 30 insertions(+), 3 deletions(-) > > Hi Liu, > > This is a good test, but we should also add one with a parser returning a value > that is not skb->len. This doesn't cover the case fixed in patch 1/3 correct? > For that we would need to modify the BPF prog itself as well > sockmap_parse_prog.c. > Hi John, This test patch use tools/testing/selftests/bpf/progs/test_sockmap_kern.c not sockmap_parse_prog.c. If we set skb_use_parser to nonzero, the bpf parser program will return skb_use_parser not skb->len. In this test case, I set skb_use_parser to 10, skb->len to 20 (opt->iov_length). This can test 1/3 patch, it will check the recved data len is 10 not 20. The parser prog in tools/testing/selftests/bpf/progs/test_sockmap_kern.h SEC("sk_skb1") int bpf_prog1(struct __sk_buff *skb) { int *f, two = 2; f = bpf_map_lookup_elem(&sock_skb_opts, &two); if (f && *f) { return *f; } return skb->len; } > For this patch though, > > Acked-by: John Fastabend <john.fastabend@gmail.com> > > Then one more patch is all we need something to break up the skb from the > parser. We really need the test because its not something we can easily test > otherwise and I don't have any use cases that do this so wouldn't catch it. > > Thanks! > John
> -----Original Message----- > From: liujian (CE) > Sent: Monday, October 25, 2021 6:17 PM > To: 'John Fastabend' <john.fastabend@gmail.com>; daniel@iogearbox.net; > jakub@cloudflare.com; lmb@cloudflare.com; davem@davemloft.net; > kuba@kernel.org; ast@kernel.org; andrii@kernel.org; kafai@fb.com; > songliubraving@fb.com; yhs@fb.com; kpsingh@kernel.org; > netdev@vger.kernel.org; bpf@vger.kernel.org; xiyou.wangcong@gmail.com > Subject: RE: [PATHC bpf v5 3/3] selftests, bpf: Add one test for sockmap with > strparser > > > > > -----Original Message----- > > From: John Fastabend [mailto:john.fastabend@gmail.com] > > Sent: Friday, October 22, 2021 11:31 PM > > To: liujian (CE) <liujian56@huawei.com>; john.fastabend@gmail.com; > > daniel@iogearbox.net; jakub@cloudflare.com; lmb@cloudflare.com; > > davem@davemloft.net; kuba@kernel.org; ast@kernel.org; > > andrii@kernel.org; kafai@fb.com; songliubraving@fb.com; yhs@fb.com; > > kpsingh@kernel.org; netdev@vger.kernel.org; bpf@vger.kernel.org; > > xiyou.wangcong@gmail.com > > Cc: liujian (CE) <liujian56@huawei.com> > > Subject: RE: [PATHC bpf v5 3/3] selftests, bpf: Add one test for > > sockmap with strparser > > > > Liu Jian wrote: > > > Add the test to check sockmap with strparser is working well. > > > > > > Signed-off-by: Liu Jian <liujian56@huawei.com> > > > --- > > > tools/testing/selftests/bpf/test_sockmap.c | 33 > > > ++++++++++++++++++++-- > > > 1 file changed, 30 insertions(+), 3 deletions(-) > > > > Hi Liu, > > > > This is a good test, but we should also add one with a parser > > returning a value that is not skb->len. This doesn't cover the case fixed in > patch 1/3 correct? > > For that we would need to modify the BPF prog itself as well > > sockmap_parse_prog.c. > > > Hi John, > This test patch use tools/testing/selftests/bpf/progs/test_sockmap_kern.c > not sockmap_parse_prog.c. > > If we set skb_use_parser to nonzero, the bpf parser program will return > skb_use_parser not skb->len. > In this test case, I set skb_use_parser to 10, skb->len to 20 (opt->iov_length). > This can test 1/3 patch, it will check the recved data len is 10 not 20. > Sorry, it will check the recved data length is 20 not 40. > The parser prog in tools/testing/selftests/bpf/progs/test_sockmap_kern.h > SEC("sk_skb1") > int bpf_prog1(struct __sk_buff *skb) > { > int *f, two = 2; > > f = bpf_map_lookup_elem(&sock_skb_opts, &two); > if (f && *f) { > return *f; > } > return skb->len; > } > > For this patch though, > > > > Acked-by: John Fastabend <john.fastabend@gmail.com> > > > > Then one more patch is all we need something to break up the skb from > > the parser. We really need the test because its not something we can > > easily test otherwise and I don't have any use cases that do this so wouldn't > catch it. > > > > Thanks! > > John
diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c index 06924917ad77..1ba7e7346afb 100644 --- a/tools/testing/selftests/bpf/test_sockmap.c +++ b/tools/testing/selftests/bpf/test_sockmap.c @@ -139,6 +139,7 @@ struct sockmap_options { bool sendpage; bool data_test; bool drop_expected; + bool check_recved_len; int iov_count; int iov_length; int rate; @@ -556,8 +557,12 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt, int err, i, flags = MSG_NOSIGNAL; bool drop = opt->drop_expected; bool data = opt->data_test; + int iov_alloc_length = iov_length; - err = msg_alloc_iov(&msg, iov_count, iov_length, data, tx); + if (!tx && opt->check_recved_len) + iov_alloc_length *= 2; + + err = msg_alloc_iov(&msg, iov_count, iov_alloc_length, data, tx); if (err) goto out_errno; if (peek_flag) { @@ -665,6 +670,13 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt, s->bytes_recvd += recv; + if (opt->check_recved_len && s->bytes_recvd > total_bytes) { + errno = EMSGSIZE; + fprintf(stderr, "recv failed(), bytes_recvd:%zd, total_bytes:%f\n", + s->bytes_recvd, total_bytes); + goto out_errno; + } + if (data) { int chunk_sz = opt->sendpage ? iov_length * cnt : @@ -744,7 +756,8 @@ static int sendmsg_test(struct sockmap_options *opt) rxpid = fork(); if (rxpid == 0) { - iov_buf -= (txmsg_pop - txmsg_start_pop + 1); + if (txmsg_pop || txmsg_start_pop) + iov_buf -= (txmsg_pop - txmsg_start_pop + 1); if (opt->drop_expected || txmsg_ktls_skb_drop) _exit(0); @@ -1688,6 +1701,19 @@ static void test_txmsg_ingress_parser(int cgrp, struct sockmap_options *opt) test_exec(cgrp, opt); } +static void test_txmsg_ingress_parser2(int cgrp, struct sockmap_options *opt) +{ + if (ktls == 1) + return; + skb_use_parser = 10; + opt->iov_length = 20; + opt->iov_count = 1; + opt->rate = 1; + opt->check_recved_len = true; + test_exec(cgrp, opt); + opt->check_recved_len = false; +} + char *map_names[] = { "sock_map", "sock_map_txmsg", @@ -1786,7 +1812,8 @@ struct _test test[] = { {"txmsg test pull-data", test_txmsg_pull}, {"txmsg test pop-data", test_txmsg_pop}, {"txmsg test push/pop data", test_txmsg_push_pop}, - {"txmsg text ingress parser", test_txmsg_ingress_parser}, + {"txmsg test ingress parser", test_txmsg_ingress_parser}, + {"txmsg test ingress parser2", test_txmsg_ingress_parser2}, }; static int check_whitelist(struct _test *t, struct sockmap_options *opt)
Add the test to check sockmap with strparser is working well. Signed-off-by: Liu Jian <liujian56@huawei.com> --- tools/testing/selftests/bpf/test_sockmap.c | 33 ++++++++++++++++++++-- 1 file changed, 30 insertions(+), 3 deletions(-)