* [PATCH v2 bpf-next 1/8] selftests/bpf: Add txmsg_pass to pull/push/pop in test_sockmap
2024-11-06 22:25 [PATCH v2 bpf-next 0/8] Fixes to bpf_msg_push/pop_data and test_sockmap zijianzhang
@ 2024-11-06 22:25 ` zijianzhang
2024-11-06 22:25 ` [PATCH v2 bpf-next 2/8] selftests/bpf: Fix SENDPAGE data logic " zijianzhang
` (7 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: zijianzhang @ 2024-11-06 22:25 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, edumazet, kuba,
pabeni, horms, mykolal, shuah, jakub, liujian56, cong.wang,
netdev, Zijian Zhang
From: Zijian Zhang <zijianzhang@bytedance.com>
Add txmsg_pass to test_txmsg_pull/push/pop. If txmsg_pass is missing,
tx_prog will be NULL, and no program will be attached to the sockmap.
As a result, pull/push/pop are never invoked.
Fixes: 328aa08a081b ("bpf: Selftests, break down test_sockmap into subtests")
Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
Reviewed-by: John Fastabend <john.fastabend@gmail.com>
---
tools/testing/selftests/bpf/test_sockmap.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 075c93ed143e..0f065273fde3 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -1596,11 +1596,13 @@ static void test_txmsg_cork_hangs(int cgrp, struct sockmap_options *opt)
static void test_txmsg_pull(int cgrp, struct sockmap_options *opt)
{
/* Test basic start/end */
+ txmsg_pass = 1;
txmsg_start = 1;
txmsg_end = 2;
test_send(opt, cgrp);
/* Test >4k pull */
+ txmsg_pass = 1;
txmsg_start = 4096;
txmsg_end = 9182;
test_send_large(opt, cgrp);
@@ -1629,11 +1631,13 @@ static void test_txmsg_pull(int cgrp, struct sockmap_options *opt)
static void test_txmsg_pop(int cgrp, struct sockmap_options *opt)
{
/* Test basic pop */
+ txmsg_pass = 1;
txmsg_start_pop = 1;
txmsg_pop = 2;
test_send_many(opt, cgrp);
/* Test pop with >4k */
+ txmsg_pass = 1;
txmsg_start_pop = 4096;
txmsg_pop = 4096;
test_send_large(opt, cgrp);
@@ -1662,11 +1666,13 @@ static void test_txmsg_pop(int cgrp, struct sockmap_options *opt)
static void test_txmsg_push(int cgrp, struct sockmap_options *opt)
{
/* Test basic push */
+ txmsg_pass = 1;
txmsg_start_push = 1;
txmsg_end_push = 1;
test_send(opt, cgrp);
/* Test push 4kB >4k */
+ txmsg_pass = 1;
txmsg_start_push = 4096;
txmsg_end_push = 4096;
test_send_large(opt, cgrp);
@@ -1687,6 +1693,7 @@ static void test_txmsg_push(int cgrp, struct sockmap_options *opt)
static void test_txmsg_push_pop(int cgrp, struct sockmap_options *opt)
{
+ txmsg_pass = 1;
txmsg_start_push = 1;
txmsg_end_push = 10;
txmsg_start_pop = 5;
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 bpf-next 2/8] selftests/bpf: Fix SENDPAGE data logic in test_sockmap
2024-11-06 22:25 [PATCH v2 bpf-next 0/8] Fixes to bpf_msg_push/pop_data and test_sockmap zijianzhang
2024-11-06 22:25 ` [PATCH v2 bpf-next 1/8] selftests/bpf: Add txmsg_pass to pull/push/pop in test_sockmap zijianzhang
@ 2024-11-06 22:25 ` zijianzhang
2024-11-06 22:25 ` [PATCH v2 bpf-next 3/8] selftests/bpf: Fix total_bytes in msg_loop_rx " zijianzhang
` (6 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: zijianzhang @ 2024-11-06 22:25 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, edumazet, kuba,
pabeni, horms, mykolal, shuah, jakub, liujian56, cong.wang,
netdev, Zijian Zhang
From: Zijian Zhang <zijianzhang@bytedance.com>
In the SENDPAGE test, "opt->iov_length * cnt" size of data will be sent
cnt times by sendfile.
1. In push/pop tests, they will be invoked cnt times, for the simplicity of
msg_verify_data, change chunk_sz to iov_length
2. Change iov_length in test_send_large from 1024 to 8192. We have pop test
where txmsg_start_pop is 4096. 4096 > 1024, an error will be returned.
Fixes: 328aa08a081b ("bpf: Selftests, break down test_sockmap into subtests")
Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
Reviewed-by: John Fastabend <john.fastabend@gmail.com>
---
tools/testing/selftests/bpf/test_sockmap.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 0f065273fde3..1d59bed90d80 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -420,16 +420,18 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
{
bool drop = opt->drop_expected;
unsigned char k = 0;
+ int i, j, fp;
FILE *file;
- int i, fp;
file = tmpfile();
if (!file) {
perror("create file for sendpage");
return 1;
}
- for (i = 0; i < iov_length * cnt; i++, k++)
- fwrite(&k, sizeof(char), 1, file);
+ for (i = 0; i < cnt; i++, k = 0) {
+ for (j = 0; j < iov_length; j++, k++)
+ fwrite(&k, sizeof(char), 1, file);
+ }
fflush(file);
fseek(file, 0, SEEK_SET);
@@ -623,7 +625,9 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
* This is really only useful for testing edge cases in code
* paths.
*/
- total_bytes = (float)iov_count * (float)iov_length * (float)cnt;
+ total_bytes = (float)iov_length * (float)cnt;
+ if (!opt->sendpage)
+ total_bytes *= (float)iov_count;
if (txmsg_apply)
txmsg_pop_total = txmsg_pop * (total_bytes / txmsg_apply);
else
@@ -701,7 +705,7 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
if (data) {
int chunk_sz = opt->sendpage ?
- iov_length * cnt :
+ iov_length :
iov_length * iov_count;
errno = msg_verify_data(&msg, recv, chunk_sz, &k, &bytes_cnt);
@@ -1466,8 +1470,8 @@ static void test_send_many(struct sockmap_options *opt, int cgrp)
static void test_send_large(struct sockmap_options *opt, int cgrp)
{
- opt->iov_length = 256;
- opt->iov_count = 1024;
+ opt->iov_length = 8192;
+ opt->iov_count = 32;
opt->rate = 2;
test_exec(cgrp, opt);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 bpf-next 3/8] selftests/bpf: Fix total_bytes in msg_loop_rx in test_sockmap
2024-11-06 22:25 [PATCH v2 bpf-next 0/8] Fixes to bpf_msg_push/pop_data and test_sockmap zijianzhang
2024-11-06 22:25 ` [PATCH v2 bpf-next 1/8] selftests/bpf: Add txmsg_pass to pull/push/pop in test_sockmap zijianzhang
2024-11-06 22:25 ` [PATCH v2 bpf-next 2/8] selftests/bpf: Fix SENDPAGE data logic " zijianzhang
@ 2024-11-06 22:25 ` zijianzhang
2024-11-06 22:25 ` [PATCH v2 bpf-next 4/8] selftests/bpf: Add push/pop checking for msg_verify_data " zijianzhang
` (5 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: zijianzhang @ 2024-11-06 22:25 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, edumazet, kuba,
pabeni, horms, mykolal, shuah, jakub, liujian56, cong.wang,
netdev, Zijian Zhang
From: Zijian Zhang <zijianzhang@bytedance.com>
total_bytes in msg_loop_rx should also take push into account, otherwise
total_bytes will be a smaller value, which makes the msg_loop_rx end early.
Besides, total_bytes has already taken pop into account, so we don't need
to subtract some bytes from iov_buf in sendmsg_test. The additional
subtraction may make total_bytes a negative number, and msg_loop_rx will
just end without checking anything.
Fixes: 18d4e900a450 ("bpf: Selftests, improve test_sockmap total bytes counter")
Fixes: d69672147faa ("selftests, bpf: Add one test for sockmap with strparser")
Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
Reviewed-by: John Fastabend <john.fastabend@gmail.com>
---
tools/testing/selftests/bpf/test_sockmap.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 1d59bed90d80..5f4558f1f004 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -606,8 +606,8 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
}
clock_gettime(CLOCK_MONOTONIC, &s->end);
} else {
+ float total_bytes, txmsg_pop_total, txmsg_push_total;
int slct, recvp = 0, recv, max_fd = fd;
- float total_bytes, txmsg_pop_total;
int fd_flags = O_NONBLOCK;
struct timeval timeout;
unsigned char k = 0;
@@ -628,10 +628,14 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
total_bytes = (float)iov_length * (float)cnt;
if (!opt->sendpage)
total_bytes *= (float)iov_count;
- if (txmsg_apply)
+ if (txmsg_apply) {
+ txmsg_push_total = txmsg_end_push * (total_bytes / txmsg_apply);
txmsg_pop_total = txmsg_pop * (total_bytes / txmsg_apply);
- else
+ } else {
+ txmsg_push_total = txmsg_end_push * cnt;
txmsg_pop_total = txmsg_pop * cnt;
+ }
+ total_bytes += txmsg_push_total;
total_bytes -= txmsg_pop_total;
err = clock_gettime(CLOCK_MONOTONIC, &s->start);
if (err < 0)
@@ -800,8 +804,6 @@ static int sendmsg_test(struct sockmap_options *opt)
rxpid = fork();
if (rxpid == 0) {
- if (txmsg_pop || txmsg_start_pop)
- iov_buf -= (txmsg_pop - txmsg_start_pop + 1);
if (opt->drop_expected || txmsg_ktls_skb_drop)
_exit(0);
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 bpf-next 4/8] selftests/bpf: Add push/pop checking for msg_verify_data in test_sockmap
2024-11-06 22:25 [PATCH v2 bpf-next 0/8] Fixes to bpf_msg_push/pop_data and test_sockmap zijianzhang
` (2 preceding siblings ...)
2024-11-06 22:25 ` [PATCH v2 bpf-next 3/8] selftests/bpf: Fix total_bytes in msg_loop_rx " zijianzhang
@ 2024-11-06 22:25 ` zijianzhang
2024-11-06 22:25 ` [PATCH v2 bpf-next 5/8] selftests/bpf: Add more tests for test_txmsg_push_pop " zijianzhang
` (4 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: zijianzhang @ 2024-11-06 22:25 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, edumazet, kuba,
pabeni, horms, mykolal, shuah, jakub, liujian56, cong.wang,
netdev, Zijian Zhang
From: Zijian Zhang <zijianzhang@bytedance.com>
Add push/pop checking for msg_verify_data in test_sockmap, except for
pop/push with cork tests, in these tests the logic will be different.
1. With corking, pop/push might not be invoked in each sendmsg, it makes
the layout of the received data difficult
2. It makes it hard to calculate the total_bytes in the recvmsg
Temporarily skip the data integrity test for these cases now, added a TODO
Fixes: ee9b352ce465 ("selftests/bpf: Fix msg_verify_data in test_sockmap")
Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
Reviewed-by: John Fastabend <john.fastabend@gmail.com>
---
tools/testing/selftests/bpf/test_sockmap.c | 106 ++++++++++++++++++++-
1 file changed, 101 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 5f4558f1f004..61a747afcd05 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -88,6 +88,10 @@ int ktls;
int peek_flag;
int skb_use_parser;
int txmsg_omit_skb_parser;
+int verify_push_start;
+int verify_push_len;
+int verify_pop_start;
+int verify_pop_len;
static const struct option long_options[] = {
{"help", no_argument, NULL, 'h' },
@@ -514,12 +518,41 @@ static int msg_alloc_iov(struct msghdr *msg,
return -ENOMEM;
}
-/* TODO: Add verification logic for push, pull and pop data */
+/* In push or pop test, we need to do some calculations for msg_verify_data */
+static void msg_verify_date_prep(void)
+{
+ int push_range_end = txmsg_start_push + txmsg_end_push - 1;
+ int pop_range_end = txmsg_start_pop + txmsg_pop - 1;
+
+ if (txmsg_end_push && txmsg_pop &&
+ txmsg_start_push <= pop_range_end && txmsg_start_pop <= push_range_end) {
+ /* The push range and the pop range overlap */
+ int overlap_len;
+
+ verify_push_start = txmsg_start_push;
+ verify_pop_start = txmsg_start_pop;
+ if (txmsg_start_push < txmsg_start_pop)
+ overlap_len = min(push_range_end - txmsg_start_pop + 1, txmsg_pop);
+ else
+ overlap_len = min(pop_range_end - txmsg_start_push + 1, txmsg_end_push);
+ verify_push_len = max(txmsg_end_push - overlap_len, 0);
+ verify_pop_len = max(txmsg_pop - overlap_len, 0);
+ } else {
+ /* Otherwise */
+ verify_push_start = txmsg_start_push;
+ verify_pop_start = txmsg_start_pop;
+ verify_push_len = txmsg_end_push;
+ verify_pop_len = txmsg_pop;
+ }
+}
+
static int msg_verify_data(struct msghdr *msg, int size, int chunk_sz,
- unsigned char *k_p, int *bytes_cnt_p)
+ unsigned char *k_p, int *bytes_cnt_p,
+ int *check_cnt_p, int *push_p)
{
- int i, j, bytes_cnt = *bytes_cnt_p;
+ int bytes_cnt = *bytes_cnt_p, check_cnt = *check_cnt_p, push = *push_p;
unsigned char k = *k_p;
+ int i, j;
for (i = 0, j = 0; i < msg->msg_iovlen && size; i++, j = 0) {
unsigned char *d = msg->msg_iov[i].iov_base;
@@ -538,6 +571,37 @@ static int msg_verify_data(struct msghdr *msg, int size, int chunk_sz,
}
for (; j < msg->msg_iov[i].iov_len && size; j++) {
+ if (push > 0 &&
+ check_cnt == verify_push_start + verify_push_len - push) {
+ int skipped;
+revisit_push:
+ skipped = push;
+ if (j + push >= msg->msg_iov[i].iov_len)
+ skipped = msg->msg_iov[i].iov_len - j;
+ push -= skipped;
+ size -= skipped;
+ j += skipped - 1;
+ check_cnt += skipped;
+ continue;
+ }
+
+ if (verify_pop_len > 0 && check_cnt == verify_pop_start) {
+ bytes_cnt += verify_pop_len;
+ check_cnt += verify_pop_len;
+ k += verify_pop_len;
+
+ if (bytes_cnt == chunk_sz) {
+ k = 0;
+ bytes_cnt = 0;
+ check_cnt = 0;
+ push = verify_push_len;
+ }
+
+ if (push > 0 &&
+ check_cnt == verify_push_start + verify_push_len - push)
+ goto revisit_push;
+ }
+
if (d[j] != k++) {
fprintf(stderr,
"detected data corruption @iov[%i]:%i %02x != %02x, %02x ?= %02x\n",
@@ -545,15 +609,20 @@ static int msg_verify_data(struct msghdr *msg, int size, int chunk_sz,
return -EDATAINTEGRITY;
}
bytes_cnt++;
+ check_cnt++;
if (bytes_cnt == chunk_sz) {
k = 0;
bytes_cnt = 0;
+ check_cnt = 0;
+ push = verify_push_len;
}
size--;
}
}
*k_p = k;
*bytes_cnt_p = bytes_cnt;
+ *check_cnt_p = check_cnt;
+ *push_p = push;
return 0;
}
@@ -612,6 +681,8 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
struct timeval timeout;
unsigned char k = 0;
int bytes_cnt = 0;
+ int check_cnt = 0;
+ int push = 0;
fd_set w;
fcntl(fd, fd_flags);
@@ -637,6 +708,10 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
}
total_bytes += txmsg_push_total;
total_bytes -= txmsg_pop_total;
+ if (data) {
+ msg_verify_date_prep();
+ push = verify_push_len;
+ }
err = clock_gettime(CLOCK_MONOTONIC, &s->start);
if (err < 0)
perror("recv start time");
@@ -712,7 +787,8 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
iov_length :
iov_length * iov_count;
- errno = msg_verify_data(&msg, recv, chunk_sz, &k, &bytes_cnt);
+ errno = msg_verify_data(&msg, recv, chunk_sz, &k, &bytes_cnt,
+ &check_cnt, &push);
if (errno) {
perror("data verify msg failed");
goto out_errno;
@@ -722,7 +798,9 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
recvp,
chunk_sz,
&k,
- &bytes_cnt);
+ &bytes_cnt,
+ &check_cnt,
+ &push);
if (errno) {
perror("data verify msg_peek failed");
goto out_errno;
@@ -1636,6 +1714,8 @@ static void test_txmsg_pull(int cgrp, struct sockmap_options *opt)
static void test_txmsg_pop(int cgrp, struct sockmap_options *opt)
{
+ bool data = opt->data_test;
+
/* Test basic pop */
txmsg_pass = 1;
txmsg_start_pop = 1;
@@ -1654,6 +1734,12 @@ static void test_txmsg_pop(int cgrp, struct sockmap_options *opt)
txmsg_pop = 2;
test_send_many(opt, cgrp);
+ /* TODO: Test for pop + cork should be different,
+ * - It makes the layout of the received data difficult
+ * - It makes it hard to calculate the total_bytes in the recvmsg
+ * Temporarily skip the data integrity test for this case now.
+ */
+ opt->data_test = false;
/* Test pop + cork */
txmsg_redir = 0;
txmsg_cork = 512;
@@ -1667,10 +1753,13 @@ static void test_txmsg_pop(int cgrp, struct sockmap_options *opt)
txmsg_start_pop = 1;
txmsg_pop = 2;
test_send_many(opt, cgrp);
+ opt->data_test = data;
}
static void test_txmsg_push(int cgrp, struct sockmap_options *opt)
{
+ bool data = opt->data_test;
+
/* Test basic push */
txmsg_pass = 1;
txmsg_start_push = 1;
@@ -1689,12 +1778,19 @@ static void test_txmsg_push(int cgrp, struct sockmap_options *opt)
txmsg_end_push = 2;
test_send_many(opt, cgrp);
+ /* TODO: Test for push + cork should be different,
+ * - It makes the layout of the received data difficult
+ * - It makes it hard to calculate the total_bytes in the recvmsg
+ * Temporarily skip the data integrity test for this case now.
+ */
+ opt->data_test = false;
/* Test push + cork */
txmsg_redir = 0;
txmsg_cork = 512;
txmsg_start_push = 1;
txmsg_end_push = 2;
test_send_many(opt, cgrp);
+ opt->data_test = data;
}
static void test_txmsg_push_pop(int cgrp, struct sockmap_options *opt)
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 bpf-next 5/8] selftests/bpf: Add more tests for test_txmsg_push_pop in test_sockmap
2024-11-06 22:25 [PATCH v2 bpf-next 0/8] Fixes to bpf_msg_push/pop_data and test_sockmap zijianzhang
` (3 preceding siblings ...)
2024-11-06 22:25 ` [PATCH v2 bpf-next 4/8] selftests/bpf: Add push/pop checking for msg_verify_data " zijianzhang
@ 2024-11-06 22:25 ` zijianzhang
2024-11-06 22:25 ` [PATCH v2 bpf-next 6/8] bpf, sockmap: Several fixes to bpf_msg_push_data zijianzhang
` (3 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: zijianzhang @ 2024-11-06 22:25 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, edumazet, kuba,
pabeni, horms, mykolal, shuah, jakub, liujian56, cong.wang,
netdev, Zijian Zhang
From: Zijian Zhang <zijianzhang@bytedance.com>
Add more tests for test_txmsg_push_pop in test_sockmap for better coverage
Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
Reviewed-by: John Fastabend <john.fastabend@gmail.com>
---
tools/testing/selftests/bpf/test_sockmap.c | 37 ++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 61a747afcd05..e5c7ecbe57e3 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -1795,12 +1795,49 @@ static void test_txmsg_push(int cgrp, struct sockmap_options *opt)
static void test_txmsg_push_pop(int cgrp, struct sockmap_options *opt)
{
+ /* Test push/pop range overlapping */
txmsg_pass = 1;
txmsg_start_push = 1;
txmsg_end_push = 10;
txmsg_start_pop = 5;
txmsg_pop = 4;
test_send_large(opt, cgrp);
+
+ txmsg_pass = 1;
+ txmsg_start_push = 1;
+ txmsg_end_push = 10;
+ txmsg_start_pop = 5;
+ txmsg_pop = 16;
+ test_send_large(opt, cgrp);
+
+ txmsg_pass = 1;
+ txmsg_start_push = 5;
+ txmsg_end_push = 4;
+ txmsg_start_pop = 1;
+ txmsg_pop = 10;
+ test_send_large(opt, cgrp);
+
+ txmsg_pass = 1;
+ txmsg_start_push = 5;
+ txmsg_end_push = 16;
+ txmsg_start_pop = 1;
+ txmsg_pop = 10;
+ test_send_large(opt, cgrp);
+
+ /* Test push/pop range non-overlapping */
+ txmsg_pass = 1;
+ txmsg_start_push = 1;
+ txmsg_end_push = 10;
+ txmsg_start_pop = 16;
+ txmsg_pop = 4;
+ test_send_large(opt, cgrp);
+
+ txmsg_pass = 1;
+ txmsg_start_push = 16;
+ txmsg_end_push = 10;
+ txmsg_start_pop = 5;
+ txmsg_pop = 4;
+ test_send_large(opt, cgrp);
}
static void test_txmsg_apply(int cgrp, struct sockmap_options *opt)
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 bpf-next 6/8] bpf, sockmap: Several fixes to bpf_msg_push_data
2024-11-06 22:25 [PATCH v2 bpf-next 0/8] Fixes to bpf_msg_push/pop_data and test_sockmap zijianzhang
` (4 preceding siblings ...)
2024-11-06 22:25 ` [PATCH v2 bpf-next 5/8] selftests/bpf: Add more tests for test_txmsg_push_pop " zijianzhang
@ 2024-11-06 22:25 ` zijianzhang
2024-11-06 22:25 ` [PATCH v2 bpf-next 7/8] bpf, sockmap: Several fixes to bpf_msg_pop_data zijianzhang
` (2 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: zijianzhang @ 2024-11-06 22:25 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, edumazet, kuba,
pabeni, horms, mykolal, shuah, jakub, liujian56, cong.wang,
netdev, Zijian Zhang
From: Zijian Zhang <zijianzhang@bytedance.com>
Several fixes to bpf_msg_push_data,
1. test_sockmap has tests where bpf_msg_push_data is invoked to push some
data at the end of a message, but -EINVAL is returned. In this case, in
bpf_msg_push_data, after the first loop, i will be set to msg->sg.end, add
the logic to handle it.
2. In the code block of "if (start - offset)", it's possible that "i"
points to the last of sk_msg_elem. In this case, "sk_msg_iter_next(msg,
end)" might still be called twice, another invoking is in "if (!copy)"
code block, but actually only one is needed. Add the logic to handle it,
and reconstruct the code to make the logic more clear.
Fixes: 6fff607e2f14 ("bpf: sk_msg program helper bpf_msg_push_data")
Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
---
net/core/filter.c | 53 +++++++++++++++++++++++++++++------------------
1 file changed, 33 insertions(+), 20 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index a88e6924c4c0..4fae427aa5ca 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2793,7 +2793,7 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
sk_msg_iter_var_next(i);
} while (i != msg->sg.end);
- if (start >= offset + l)
+ if (start > offset + l)
return -EINVAL;
space = MAX_MSG_FRAGS - sk_msg_elem_used(msg);
@@ -2818,6 +2818,8 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
raw = page_address(page);
+ if (i == msg->sg.end)
+ sk_msg_iter_var_prev(i);
psge = sk_msg_elem(msg, i);
front = start - offset;
back = psge->length - front;
@@ -2834,7 +2836,13 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
}
put_page(sg_page(psge));
- } else if (start - offset) {
+ new = i;
+ goto place_new;
+ }
+
+ if (start - offset) {
+ if (i == msg->sg.end)
+ sk_msg_iter_var_prev(i);
psge = sk_msg_elem(msg, i);
rsge = sk_msg_elem_cpy(msg, i);
@@ -2845,39 +2853,44 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
sk_msg_iter_var_next(i);
sg_unmark_end(psge);
sg_unmark_end(&rsge);
- sk_msg_iter_next(msg, end);
}
/* Slot(s) to place newly allocated data */
+ sk_msg_iter_next(msg, end);
new = i;
+ sk_msg_iter_var_next(i);
+
+ if (i == msg->sg.end) {
+ if (!rsge.length)
+ goto place_new;
+ sk_msg_iter_next(msg, end);
+ goto place_new;
+ }
/* Shift one or two slots as needed */
- if (!copy) {
- sge = sk_msg_elem_cpy(msg, i);
+ sge = sk_msg_elem_cpy(msg, new);
+ sg_unmark_end(&sge);
+ nsge = sk_msg_elem_cpy(msg, i);
+ if (rsge.length) {
sk_msg_iter_var_next(i);
- sg_unmark_end(&sge);
+ nnsge = sk_msg_elem_cpy(msg, i);
sk_msg_iter_next(msg, end);
+ }
- nsge = sk_msg_elem_cpy(msg, i);
+ while (i != msg->sg.end) {
+ msg->sg.data[i] = sge;
+ sge = nsge;
+ sk_msg_iter_var_next(i);
if (rsge.length) {
- sk_msg_iter_var_next(i);
+ nsge = nnsge;
nnsge = sk_msg_elem_cpy(msg, i);
- }
-
- while (i != msg->sg.end) {
- msg->sg.data[i] = sge;
- sge = nsge;
- sk_msg_iter_var_next(i);
- if (rsge.length) {
- nsge = nnsge;
- nnsge = sk_msg_elem_cpy(msg, i);
- } else {
- nsge = sk_msg_elem_cpy(msg, i);
- }
+ } else {
+ nsge = sk_msg_elem_cpy(msg, i);
}
}
+place_new:
/* Place newly allocated data buffer */
sk_mem_charge(msg->sk, len);
msg->sg.size += len;
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 bpf-next 7/8] bpf, sockmap: Several fixes to bpf_msg_pop_data
2024-11-06 22:25 [PATCH v2 bpf-next 0/8] Fixes to bpf_msg_push/pop_data and test_sockmap zijianzhang
` (5 preceding siblings ...)
2024-11-06 22:25 ` [PATCH v2 bpf-next 6/8] bpf, sockmap: Several fixes to bpf_msg_push_data zijianzhang
@ 2024-11-06 22:25 ` zijianzhang
2024-11-06 22:25 ` [PATCH v2 bpf-next 8/8] bpf, sockmap: Fix sk_msg_reset_curr zijianzhang
2024-11-07 0:30 ` [PATCH v2 bpf-next 0/8] Fixes to bpf_msg_push/pop_data and test_sockmap patchwork-bot+netdevbpf
8 siblings, 0 replies; 12+ messages in thread
From: zijianzhang @ 2024-11-06 22:25 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, edumazet, kuba,
pabeni, horms, mykolal, shuah, jakub, liujian56, cong.wang,
netdev, Zijian Zhang
From: Zijian Zhang <zijianzhang@bytedance.com>
Several fixes to bpf_msg_pop_data,
1. In sk_msg_shift_left, we should put_page
2. if (len == 0), return early is better
3. pop the entire sk_msg (last == msg->sg.size) should be supported
4. Fix for the value of variable "a"
5. In sk_msg_shift_left, after shifting, i has already pointed to the next
element. Addtional sk_msg_iter_var_next may result in BUG.
Fixes: 7246d8ed4dcc ("bpf: helper to pop data from messages")
Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
Reviewed-by: John Fastabend <john.fastabend@gmail.com>
---
net/core/filter.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 4fae427aa5ca..fba445b96de8 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2919,8 +2919,10 @@ static const struct bpf_func_proto bpf_msg_push_data_proto = {
static void sk_msg_shift_left(struct sk_msg *msg, int i)
{
+ struct scatterlist *sge = sk_msg_elem(msg, i);
int prev;
+ put_page(sg_page(sge));
do {
prev = i;
sk_msg_iter_var_next(i);
@@ -2957,6 +2959,9 @@ BPF_CALL_4(bpf_msg_pop_data, struct sk_msg *, msg, u32, start,
if (unlikely(flags))
return -EINVAL;
+ if (unlikely(len == 0))
+ return 0;
+
/* First find the starting scatterlist element */
i = msg->sg.start;
do {
@@ -2969,7 +2974,7 @@ BPF_CALL_4(bpf_msg_pop_data, struct sk_msg *, msg, u32, start,
} while (i != msg->sg.end);
/* Bounds checks: start and pop must be inside message */
- if (start >= offset + l || last >= msg->sg.size)
+ if (start >= offset + l || last > msg->sg.size)
return -EINVAL;
space = MAX_MSG_FRAGS - sk_msg_elem_used(msg);
@@ -2998,12 +3003,12 @@ BPF_CALL_4(bpf_msg_pop_data, struct sk_msg *, msg, u32, start,
*/
if (start != offset) {
struct scatterlist *nsge, *sge = sk_msg_elem(msg, i);
- int a = start;
+ int a = start - offset;
int b = sge->length - pop - a;
sk_msg_iter_var_next(i);
- if (pop < sge->length - a) {
+ if (b > 0) {
if (space) {
sge->length = a;
sk_msg_shift_right(msg, i);
@@ -3022,7 +3027,6 @@ BPF_CALL_4(bpf_msg_pop_data, struct sk_msg *, msg, u32, start,
if (unlikely(!page))
return -ENOMEM;
- sge->length = a;
orig = sg_page(sge);
from = sg_virt(sge);
to = page_address(page);
@@ -3032,7 +3036,7 @@ BPF_CALL_4(bpf_msg_pop_data, struct sk_msg *, msg, u32, start,
put_page(orig);
}
pop = 0;
- } else if (pop >= sge->length - a) {
+ } else {
pop -= (sge->length - a);
sge->length = a;
}
@@ -3066,7 +3070,6 @@ BPF_CALL_4(bpf_msg_pop_data, struct sk_msg *, msg, u32, start,
pop -= sge->length;
sk_msg_shift_left(msg, i);
}
- sk_msg_iter_var_next(i);
}
sk_mem_uncharge(msg->sk, len - pop);
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 bpf-next 8/8] bpf, sockmap: Fix sk_msg_reset_curr
2024-11-06 22:25 [PATCH v2 bpf-next 0/8] Fixes to bpf_msg_push/pop_data and test_sockmap zijianzhang
` (6 preceding siblings ...)
2024-11-06 22:25 ` [PATCH v2 bpf-next 7/8] bpf, sockmap: Several fixes to bpf_msg_pop_data zijianzhang
@ 2024-11-06 22:25 ` zijianzhang
2024-11-07 0:30 ` [PATCH v2 bpf-next 0/8] Fixes to bpf_msg_push/pop_data and test_sockmap patchwork-bot+netdevbpf
8 siblings, 0 replies; 12+ messages in thread
From: zijianzhang @ 2024-11-06 22:25 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, edumazet, kuba,
pabeni, horms, mykolal, shuah, jakub, liujian56, cong.wang,
netdev, Zijian Zhang
From: Zijian Zhang <zijianzhang@bytedance.com>
Found in the test_txmsg_pull in test_sockmap,
```
txmsg_cork = 512; // corking is importrant here
opt->iov_length = 3;
opt->iov_count = 1;
opt->rate = 512; // sendmsg will be invoked 512 times
```
The first sendmsg will send an sk_msg with size 3, and bpf_msg_pull_data
will be invoked the first time. sk_msg_reset_curr will reset the copybreak
from 3 to 0. In the second sendmsg, since we are in the stage of corking,
psock->cork will be reused in func sk_msg_alloc. msg->sg.copybreak is 0
now, the second msg will overwrite the first msg. As a result, we could
not pass the data integrity test.
The same problem happens in push and pop test. Thus, fix sk_msg_reset_curr
to restore the correct copybreak.
Fixes: bb9aefde5bba ("bpf: sockmap, updating the sg structure should also update curr")
Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
---
net/core/filter.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index fba445b96de8..00491ac4598f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2619,18 +2619,16 @@ BPF_CALL_2(bpf_msg_cork_bytes, struct sk_msg *, msg, u32, bytes)
static void sk_msg_reset_curr(struct sk_msg *msg)
{
- u32 i = msg->sg.start;
- u32 len = 0;
-
- do {
- len += sk_msg_elem(msg, i)->length;
- sk_msg_iter_var_next(i);
- if (len >= msg->sg.size)
- break;
- } while (i != msg->sg.end);
+ if (!msg->sg.size) {
+ msg->sg.curr = msg->sg.start;
+ msg->sg.copybreak = 0;
+ } else {
+ u32 i = msg->sg.end;
- msg->sg.curr = i;
- msg->sg.copybreak = 0;
+ sk_msg_iter_var_prev(i);
+ msg->sg.curr = i;
+ msg->sg.copybreak = msg->sg.data[i].length;
+ }
}
static const struct bpf_func_proto bpf_msg_cork_bytes_proto = {
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 bpf-next 0/8] Fixes to bpf_msg_push/pop_data and test_sockmap
2024-11-06 22:25 [PATCH v2 bpf-next 0/8] Fixes to bpf_msg_push/pop_data and test_sockmap zijianzhang
` (7 preceding siblings ...)
2024-11-06 22:25 ` [PATCH v2 bpf-next 8/8] bpf, sockmap: Fix sk_msg_reset_curr zijianzhang
@ 2024-11-07 0:30 ` patchwork-bot+netdevbpf
8 siblings, 0 replies; 12+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-11-07 0:30 UTC (permalink / raw)
To: Zijian Zhang
Cc: bpf, ast, daniel, andrii, martin.lau, eddyz87, song,
yonghong.song, john.fastabend, kpsingh, sdf, haoluo, jolsa,
edumazet, kuba, pabeni, horms, mykolal, shuah, jakub, liujian56,
cong.wang, netdev
Hello:
This series was applied to bpf/bpf-next.git (net)
by Martin KaFai Lau <martin.lau@kernel.org>:
On Wed, 6 Nov 2024 22:25:12 +0000 you wrote:
> From: Zijian Zhang <zijianzhang@bytedance.com>
>
> Several fixes to test_sockmap and added push/pop logic for msg_verify_data
> Before the fixes, some of the tests in test_sockmap are problematic,
> resulting in pseudo-correct result.
>
> 1. txmsg_pass is not set in some tests, as a result, no eBPF program is
> attached to the sockmap.
> 2. In SENDPAGE, a wrong iov_length in test_send_large may result in some
> test skippings and failures.
> 3. The calculation of total_bytes in msg_loop_rx is wrong, which may cause
> msg_loop_rx end early and skip some data tests.
>
> [...]
Here is the summary with links:
- [v2,bpf-next,1/8] selftests/bpf: Add txmsg_pass to pull/push/pop in test_sockmap
https://git.kernel.org/bpf/bpf-next/c/66c54c20408d
- [v2,bpf-next,2/8] selftests/bpf: Fix SENDPAGE data logic in test_sockmap
https://git.kernel.org/bpf/bpf-next/c/4095031463d4
- [v2,bpf-next,3/8] selftests/bpf: Fix total_bytes in msg_loop_rx in test_sockmap
https://git.kernel.org/bpf/bpf-next/c/523dffccbade
- [v2,bpf-next,4/8] selftests/bpf: Add push/pop checking for msg_verify_data in test_sockmap
https://git.kernel.org/bpf/bpf-next/c/862087c3d362
- [v2,bpf-next,5/8] selftests/bpf: Add more tests for test_txmsg_push_pop in test_sockmap
https://git.kernel.org/bpf/bpf-next/c/47eae080410b
- [v2,bpf-next,6/8] bpf, sockmap: Several fixes to bpf_msg_push_data
https://git.kernel.org/bpf/bpf-next/c/15ab0548e310
- [v2,bpf-next,7/8] bpf, sockmap: Several fixes to bpf_msg_pop_data
https://git.kernel.org/bpf/bpf-next/c/5d609ba26247
- [v2,bpf-next,8/8] bpf, sockmap: Fix sk_msg_reset_curr
https://git.kernel.org/bpf/bpf-next/c/955afd57dc4b
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 12+ messages in thread