On Wed, 21 Aug 2024 20:54:49 -0400
Jon Maloy <jmaloy(a)redhat.com> wrote:
We add a selftest to check that the SO_PEEK_OFF
feature
recently added to tcp works correctly.
I would also mention the commit where you added it.
Signed-off-by: Jon Maloy <jmaloy(a)redhat.com>
---
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/net/Makefile | 1 +
tools/testing/selftests/net/tcp_so_peek_off.c | 161 ++++++++++++++++++
3 files changed, 163 insertions(+)
create mode 100644 tools/testing/selftests/net/tcp_so_peek_off.c
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index a5f1c0c27dff..70537151b59d 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -69,6 +69,7 @@ TARGETS += net/openvswitch
TARGETS += net/tcp_ao
TARGETS += net/netfilter
TARGETS += net/rds
+TARGETS += net/tcp_so_peek_off
You're not adding a new directory with a new Makefile, so you don't
need this, but:
TARGETS += nsfs
TARGETS += perf_events
TARGETS += pidfd
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 8eaffd7a641c..5dfe912e3fbb 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -63,6 +63,7 @@ TEST_GEN_FILES += tcp_mmap tcp_inq psock_snd txring_overwrite
TEST_GEN_FILES += udpgso udpgso_bench_tx udpgso_bench_rx ip_defrag
TEST_GEN_FILES += so_txtime ipv6_flowlabel ipv6_flowlabel_mgr so_netns_cookie
TEST_GEN_FILES += tcp_fastopen_backup_key
+TEST_GEN_FILES += tcp_so_peek_off
...this is not enough to make it run when kselftests ('make
kselftests') are run.
If you run 'make emit_tests' from selftests/net, which generates the
list of tests to be run, your new test won't appear there.
There are two possible approaches:
1. keep this in TEST_GEN_FILES and add a shell script, which needs to
be in TEST_PROGS, calling this
2. (preferred) add this to TEST_GEN_PROGS, which takes care of both:
your new test will be built, and also included in the set of tests
to run
TEST_GEN_FILES += fin_ack_lat
TEST_GEN_FILES += reuseaddr_ports_exhausted
TEST_GEN_FILES += hwtstamp_config rxtimestamp timestamping txtimestamp
diff --git a/tools/testing/selftests/net/tcp_so_peek_off.c
b/tools/testing/selftests/net/tcp_so_peek_off.c
new file mode 100644
index 000000000000..b4fd8a940795
--- /dev/null
+++ b/tools/testing/selftests/net/tcp_so_peek_off.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
+
+int tcp_peek_offset_probe(sa_family_t af)
+{
+ int optv = 0;
+ int ret = 0;
+ int s;
+
+ s = socket(af, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
+ if (s < 0) {
+ perror("Temporary TCP socket creation failed");
+ } else {
+ if (!setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &optv, sizeof(int)))
+ ret = 1;
+ close(s);
+ }
+ return ret;
+}
+
+
+static void tcp_peek_offset_set(int s, int offset)
+{
+ if (setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &offset, sizeof(offset)))
+ perror("Failed to set SO_PEEK_OFF value\n");
+}
+
+static int tcp_peek_offset_get(int s)
+{
+ int offset;
+ socklen_t len = sizeof(offset);
+
+ if (getsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &offset, &len))
+ perror("Failed to get SO_PEEK_OFF value\n");
+ return offset;
+}
+
+static int tcp_peek_offset_test(void)
+{
+ struct sockaddr a = { AF_INET, {0, }};
+ int err = EXIT_FAILURE;
+ int s[2] = {0, 0};
+ int recv_sock = 0;
+ int offset = 0;
+ ssize_t len;
+ char buf;
+
+ s[0] = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
+ s[1] = socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP);
+
+ if (s[0] < 0 || s[1] < 0) {
+ perror("Temporary probe socket creation failed\n");
+ goto out;
+ }
+ if (0 > bind(s[0], &a, sizeof(a))) {
I'm not sure if this is actively discouraged, but I've never seen this
style of comparisons (0 > ...) in the kernel (at least not in
networking code).
I heard somebody suggesting it in university courses (perhaps?) "so
that you won't mix up = and ==", but in my opinion it just makes things
unnatural and less readable.
+ perror("Temporary probe socket bind()
failed\n");
+ goto out;
+ }
+ if (0 > getsockname(s[0], &a, &((socklen_t) { sizeof(a) }))) {
+ perror("Temporary probe socket getsockname() failed\n");
+ goto out;
+ }
+ if (0 > listen(s[0], 0)) {
+ perror("Temporary probe socket listen() failed\n");
+ goto out;
+ }
+ if (0 <= connect(s[1], &a, sizeof(a)) || errno != EINPROGRESS) {
+ perror("Temporary probe socket connect() failed\n");
+ goto out;
+ }
+ recv_sock = accept(s[0], NULL, NULL);
+ if (recv_sock <= 0) {
+ perror("Temporary probe socket accept() failed\n");
+ goto out;
+ }
+
+ /* Some basic tests of getting/setting offset */
+ offset = tcp_peek_offset_get(recv_sock);
+ if (offset != -1) {
+ perror("Initial value of socket offset not -1\n");
+ goto out;
+ }
+ tcp_peek_offset_set(recv_sock, 0);
+ offset = tcp_peek_offset_get(recv_sock);
+ if (offset != 0) {
+ perror("Failed to set socket offset to 0\n");
+ goto out;
+ }
+
+ /* Transfer a message */
+ if (0 >= send(s[1], (char *)("ab"), 2, 0) || errno != EINPROGRESS) {
+ perror("Temporary probe socket send() failed\n");
+ goto out;
+ }
+ /* Read first byte */
+ len = recv(recv_sock, &buf, 1, MSG_PEEK);
+ if (len != 1 || buf != 'a') {
+ perror("Failed to read first byte of message\n");
+ goto out;
+ }
+ offset = tcp_peek_offset_get(recv_sock);
+ if (offset != 1) {
+ perror("Offset not forwarded correctly at first byte\n");
+ goto out;
+ }
+ /* Try to read beyond last byte */
+ len = recv(recv_sock, &buf, 2, MSG_PEEK);
+ if (len != 1 || buf != 'b') {
+ perror("Failed to read last byte of message\n");
+ goto out;
+ }
+ offset = tcp_peek_offset_get(recv_sock);
+ if (offset != 2) {
+ perror("Offset not forwarded correctly at last byte\n");
+ goto out;
+ }
+ /* Flush message */
+ len = recv(recv_sock, NULL, 2, MSG_TRUNC);
+ if (len != 2) {
+ perror("Failed to flush message\n");
+ goto out;
+ }
+ offset = tcp_peek_offset_get(recv_sock);
+ if (offset != 0) {
+ perror("Offset not reverted correctly after flush\n");
+ goto out;
+ }
+
+ printf("TCP with MSG_PEEK_OFF works correctly\n");
+ err = EXIT_SUCCESS;
This should be KSFT_PASS, it comes from "../kselftest.h".
By the way, it would be nice to use the functions and macros there to
print messages (see the comment at the beginning of that file), even
though I don't see many "net" tests doing that.
+out:
+ close(recv_sock);
+ close(s[1]);
+ close(s[0]);
+ return err;
...but if there's an error, you need to return KSFT_FAIL from main()
(or here).
+}
+
+int main(int argc, char *argv[])
You don't use argc and argv, so you could just use main() (gcc's -Wextra
warns... I know it's not on by default here though).
+{
+ int cap4 = 0, cap6 = 0;
+
+ cap4 = tcp_peek_offset_probe(AF_INET);
+ if (!cap4)
+ printf("SO_PEEK_OFF not supported for TCP/IPv4\n");
+ cap6 = tcp_peek_offset_probe(AF_INET6);
+ if (!cap6)
+ printf("SO_PEEK_OFF not supported for TCP/IPv6\n");
I don't have a machine running a kernel supporting this -- note that:
https://patchwork.kernel.org/project/netdevbpf/patch/20240720140914.2772902…
wasn't applied (you should include the two patches in a series I guess).
But you're just testing AF_INET sockets in tcp_peek_offset_test(), so I
wonder why you're checking for IPv6 support first. Would it perhaps
make sense to test IPv4 first (probe and actual test), then do the same
with IPv6?
+ if (!cap4 || !cap6)
+ exit(EXIT_SUCCESS);
This shouldn't be EXIT_SUCCESS, it should be KSFT_SKIP.
+
+ exit(tcp_peek_offset_test());
+}
+
Other than that, the logic looks good to me, and the IPv4 part works
for me on a 6.10 kernel.
--
Stefano