Re: selftests: add selftest for tcp SO_PEEK_OFF support

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 21 Aug 2024 20:54:49 -0400
Jon Maloy <jmaloy@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@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-1-jmaloy@redhat.com/

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