Various fixes for sockmap and reuseport tests

Use a proper temporary file for sendpage tests. This means that running the tests doesn't clutter the working directory, and allows running the test on read-only filesystems.

Signed-off-by Lorenz Bauer <[email protected]>

---
 tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

tools/testing/selftests/bpf/test_sockmap.c +1 -1

index 4a851513c842..779e11da979c 100644
@@ -331,7 +331,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
 	FILE *file;
 	int i, fp;
 
-	file = fopen(".sendpage_tst.tmp", "w+");
+	file = tmpfile();
 	if (!file) {
 		perror("create file for sendpage");
 		return 1;

@@ -340,13 +340,8 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
 		fwrite(&k, sizeof(char), 1, file);

tools/testing/selftests/bpf/test_sockmap.c +0 -0

 	clock_gettime(CLOCK_MONOTONIC, &s->start);
 	for (i = 0; i < cnt; i++) {

Quoted 28 lines · Lorenz Bauer

Use a proper temporary file for sendpage tests. This means that running the tests doesn't clutter the working directory, and allows running the test on read-only filesystems.

Signed-off-by Lorenz Bauer <[email protected]>

---
 tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

tools/testing/selftests/bpf/test_sockmap.c +1 -2

index 4a851513c842..779e11da979c 100644
@@ -331,7 +331,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
 	FILE *file;
 	int i, fp;
 
-	file = fopen(".sendpage_tst.tmp", "w+");
+	file = tmpfile();
 	if (!file) {
 		perror("create file for sendpage");
 		return 1;

@@ -340,13 +340,8 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
 		fwrite(&k, sizeof(char), 1, file);
 	fflush(file);
 	fseek(file, 0, SEEK_SET);
-	fclose(file);
 

tools/testing/selftests/bpf/test_sockmap.c static int msg_loop_sendpage(int fd, int iov_length, int cnt, · line 344

Quoted 6 lines

tools/testing/selftests/bpf/test_sockmap.c +1 -5

-	fp = open(".sendpage_tst.tmp", O_RDONLY);
-	if (fp < 0) {
-		perror("reopen file for sendpage");
-		return 1;
-	}
+	fp = fileno(file);

It may be better to keep fp == -1 check here. It is not clear to me the original intention of reopen. I would defer to John for comment.

Quoted 3 lines

tools/testing/selftests/bpf/test_sockmap.c +0 -0

 	clock_gettime(CLOCK_MONOTONIC, &s->start);
 	for (i = 0; i < cnt; i++) {

Use a proper temporary file for sendpage tests. This means that running the tests doesn't clutter the working directory, and allows running the test on read-only filesystems.

Signed-off-by Lorenz Bauer <[email protected]>

---
 tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

tools/testing/selftests/bpf/test_sockmap.c +0 -0

index 4a851513c842..779e11da979c 100644

Use a proper temporary file for sendpage tests. This means that running the tests doesn't clutter the working directory, and allows running the test on read-only filesystems.

Signed-off-by Lorenz Bauer <[email protected]>

---
 tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

The reuseport tests currently suffer from a race condition: RST packets count towards DROP_ERR_SKB_DATA, since they don't contain a valid struct cmd. Tests will spuriously fail depending on whether check_results is called before or after the RST is processed.

Exit the BPF program early if FIN is set.

Quoted 6 lines · Lorenz Bauer

The reuseport tests currently suffer from a race condition: RST packets count towards DROP_ERR_SKB_DATA, since they don't contain a valid struct cmd. Tests will spuriously fail depending on whether check_results is called before or after the RST is processed.

Exit the BPF program early if FIN is set.

Make sense.
Is it a RST or FIN? The earlier commit message said RST.

The reuseport tests currently suffer from a race condition: RST packets count towards DROP_ERR_SKB_DATA, since they don't contain a valid struct cmd. Tests will spuriously fail depending on whether check_results is called before or after the RST is processed.

Exit the BPF program early if FIN is set.

btw, it needs a Fixes tag.

Patch 4 and Patch 1 also need a Fixes tag.

Quoted 4 lines · Martin Lau

btw, it needs a Fixes tag.

Patch 4 and Patch 1 also need a Fixes tag.

This makes me wonder, should these go via bpf or bpf-next? Do I have to split the series then?

The reuseport tests currently suffer from a race condition: RST packets count towards DROP_ERR_SKB_DATA, since they don't contain a valid struct cmd. Tests will spuriously fail depending on whether check_results is called before or after the RST is processed.

Exit the BPF program early if FIN is set.

Signed-off-by Lorenz Bauer <[email protected]>

---
 .../selftests/bpf/progs/test_select_reuseport_kern.c        | 6 ++++++
 1 file changed, 6 insertions(+)

tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c +6 -0

index d69a1f2bbbfd..26e77dcc7e91 100644
@@ -113,6 +113,12 @@ int _select_by_skb_data(struct sk_reuseport_md *reuse_md)
 		data_check.skb_ports[0] = th->source;
 		data_check.skb_ports[1] = th->dest;
 
+		if (th->fin)
+			/* The connection is being torn down at the end of a
+			 * test. It can't contain a cmd, so return early.
+			 */
+			return SK_PASS;
+
 		if ((th->doff << 2) + sizeof(*cmd) > data_check.len)
 			GOTO_DONE(DROP_ERR_SKB_DATA);
 		if (bpf_skb_load_bytes(reuse_md, th->doff << 2, &cmd_copy,

Signature · 1 line

2.20.1

Include the name of the mismatching result in human readable format when reporting an error. The new output looks like the following:

unexpected result
result: [1, 0, 0, 0, 0, 0]
expected: [0, 0, 0, 0, 0, 0]
mismatch on DROP_ERR_INNER_MAP (bpf_prog_linum:153) check_results:FAIL:382

Signed-off-by Lorenz Bauer <[email protected]>

---
 .../bpf/prog_tests/select_reuseport.c         | 30 ++++++++++++++++---
 1 file changed, 26 insertions(+), 4 deletions(-)

tools/testing/selftests/bpf/prog_tests/select_reuseport.c +6 -0

index 2c37ae7dc214..09a536af139a 100644
@@ -316,6 +316,28 @@ static void check_data(int type, sa_family_t family, const struct cmd *cmd,
 		       expected.len, result.len, get_linum());
 }
 
+static const char *result_to_str(enum result res)
+{
+	switch (res) {
+	case DROP_ERR_INNER_MAP:
+		return "DROP_ERR_INNER_MAP";
+	case DROP_ERR_SKB_DATA:

tools/testing/selftests/bpf/prog_tests/select_reuseport.c +6 -0

+		return "NR_RESULTS";
+	default:
+		return "UNKNOWN";
+	}
+}
+

Include the name of the mismatching result in human readable format when reporting an error. The new output looks like the following:

unexpected result
result: [1, 0, 0, 0, 0, 0]
expected: [0, 0, 0, 0, 0, 0]
mismatch on DROP_ERR_INNER_MAP (bpf_prog_linum:153) check_results:FAIL:382

Signed-off-by Lorenz Bauer <[email protected]>

---
 .../bpf/prog_tests/select_reuseport.c         | 30 ++++++++++++++++---
 1 file changed, 26 insertions(+), 4 deletions(-)

tools/testing/selftests/bpf/prog_tests/select_reuseport.c +26 -4

index 2c37ae7dc214..09a536af139a 100644
@@ -316,6 +316,28 @@ static void check_data(int type, sa_family_t family, const struct cmd *cmd,
 		       expected.len, result.len, get_linum());
 }
 
+static const char *result_to_str(enum result res)
+{
+	switch (res) {
+	case DROP_ERR_INNER_MAP:
+		return "DROP_ERR_INNER_MAP";
+	case DROP_ERR_SKB_DATA:
+		return "DROP_ERR_SKB_DATA";
+	case DROP_ERR_SK_SELECT_REUSEPORT:
+		return "DROP_ERR_SK_SELECT_REUSEPORT";
+	case DROP_MISC:
+		return "DROP_MISC";
+	case PASS:
+		return "PASS";
+	case PASS_ERR_SK_SELECT_REUSEPORT:
+		return "PASS_ERR_SK_SELECT_REUSEPORT";
+	case NR_RESULTS:
+		return "NR_RESULTS";
+	default:
+		return "UNKNOWN";
+	}
+}
+
 static void check_results(void)
 {
 	__u32 results[NR_RESULTS];

@@ -351,10 +373,10 @@ static void check_results(void)
 		printf(", %u", expected_results[i]);
 	printf("]\n");
 
-	RET_IF(expected_results[broken] != results[broken],
-	       "unexpected result",
-	       "expected_results[%u] != results[%u] bpf_prog_linum:%ld\n",
-	       broken, broken, get_linum());
+	printf("mismatch on %s (bpf_prog_linum:%ld)\n", result_to_str(broken),
+	       get_linum());
+
+	CHECK_FAIL(true);
 }
 
 static int send_data(int type, sa_family_t family, void *data, size_t len,

Signature · 1 line

2.20.1

Currently, there is a lot of false positives if a single reuseport test fails. This is because expected_results and the result map are not cleared.

Zero both after individual test runs, which fixes the mentioned false positives.

I've fixed the commit messages, added Fixes tags and am submitting to bpf-next instead of the bpf tree.

There is still the question whether patch #1 needs to preserve O_RDONLY, which John can hopefully answer.

Lorenz Bauer (4):
selftests: bpf: use a temporary file in test_sockmap
selftests: bpf: ignore FIN packets for reuseport tests
selftests: bpf: make reuseport test output more legible
selftests: bpf: reset global state between reuseport test runs

 .../bpf/prog_tests/select_reuseport.c         | 44 ++++++++++++++++---
 .../bpf/progs/test_select_reuseport_kern.c    |  6 +++
 tools/testing/selftests/bpf/test_sockmap.c    | 15 +++----
 3 files changed, 49 insertions(+), 16 deletions(-)

I've fixed the commit messages, added Fixes tags and am submitting to bpf-next instead of the bpf tree.

Quoted 2 lines · Lorenz Bauer

I've fixed the commit messages, added Fixes tags and am submitting to bpf-next instead of the bpf tree.

Acked-by Martin KaFai Lau <[email protected]>