> -----Original Message----- > From: liujian (CE) > Sent: Monday, October 4, 2021 12:28 PM > To: 'John Fastabend' <john.fastabend@xxxxxxxxx>; daniel@xxxxxxxxxxxxx; > jakub@xxxxxxxxxxxxxx; lmb@xxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; > kuba@xxxxxxxxxx; ast@xxxxxxxxxx; andrii@xxxxxxxxxx; kafai@xxxxxx; > songliubraving@xxxxxx; yhs@xxxxxx; kpsingh@xxxxxxxxxx; > netdev@xxxxxxxxxxxxxxx; bpf@xxxxxxxxxxxxxxx; xiyou.wangcong@xxxxxxxxx > Subject: RE: [PATCH v4] skmsg: lose offset info in sk_psock_skb_ingress > > > > > -----Original Message----- > > From: John Fastabend [mailto:john.fastabend@xxxxxxxxx] > > Sent: Friday, October 1, 2021 6:48 AM > > To: liujian (CE) <liujian56@xxxxxxxxxx>; john.fastabend@xxxxxxxxx; > > daniel@xxxxxxxxxxxxx; jakub@xxxxxxxxxxxxxx; lmb@xxxxxxxxxxxxxx; > > davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; ast@xxxxxxxxxx; > > andrii@xxxxxxxxxx; kafai@xxxxxx; songliubraving@xxxxxx; yhs@xxxxxx; > > kpsingh@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; bpf@xxxxxxxxxxxxxxx; > > xiyou.wangcong@xxxxxxxxx > > Cc: liujian (CE) <liujian56@xxxxxxxxxx> > > Subject: RE: [PATCH v4] skmsg: lose offset info in > > sk_psock_skb_ingress > > > > Liu Jian wrote: > > > If sockmap enable strparser, there are lose offset info in > > > sk_psock_skb_ingress. If the length determined by parse_msg function > > > is not skb->len, the skb will be converted to sk_msg multiple times, > > > and userspace app will get the data multiple times. > > > > > > Fix this by get the offset and length from strp_msg. > > > And as Cong suggestion, add one bit in skb->_sk_redir to distinguish > > > enable or disable strparser. > > > > > > Signed-off-by: Liu Jian <liujian56@xxxxxxxxxx> > > > --- > > > > Thanks. Please add Fixes tags so we can track these I've added it here. > > > > This has been broken from the initial patches and after a quick glance > > I suspect this will need manual backports if we need it. Also all the > > I use and all the selftests set parser to a nop by returning skb->len. > > > > Can you also create a test so we can ensure we don't break this again? > Okay, I will do this after the holiday. Hi John, I checked selftests, there are have one test case named " test_txmsg_ingress_parser". But with this patch and ktls, the test failed, this because ktls parser(tls_read_size) return value is 285 not 256. the case like this: tls_sk1 --> redir_sk --> tls_sk2 tls_sk1 sent out 512 bytes data, after tls related processing redir_sk recved 570 btyes data, and redirect 512 (skb_use_parser) bytes data to tls_sk2; but tls_sk2 needs 285 * 2 bytes data, receive timeout occurred. I fix this as below: --- a/tools/testing/selftests/bpf/test_sockmap.c +++ b/tools/testing/selftests/bpf/test_sockmap.c @@ -1680,6 +1680,8 @@ static void test_txmsg_ingress_parser(int cgrp, struct sockmap_options *opt) { txmsg_pass = 1; skb_use_parser = 512; + if (ktls == 1) + skb_use_parser = 570; opt->iov_length = 256; opt->iov_count = 1; opt->rate = 2; And i add one new test as below, is it ok? --- 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}, }; > > > > Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg > > interface") > > Acked-by: John Fastabend <john.fastabend@xxxxxxxxx> > Thank you for reviewing this patch again.
