diff mbox series

[net-next,1/1] selftests: net: Add support for testing SO_RCVMARK and SO_RCVPRIORITY

Message ID 20250129143601.16035-2-annaemesenyiri@gmail.com (mailing list archive)
State New
Headers show
Series selftests: net: Add support for testing SO_RCVMARK and SO_RCVPRIORITY | expand

Commit Message

Anna Nyiri Jan. 29, 2025, 2:36 p.m. UTC 
Introduce tests to verify the correct functionality of the SO_RCVMARK and 
SO_RCVPRIORITY socket options.

Key changes include:

- so_rcv_listener.c: Implements a receiver application to test the correct 
behavior of the SO_RCVMARK and SO_RCVPRIORITY options.
- test_so_rcv.sh: Provides a shell script to automate testing for these options.
- Makefile: Integrates test_so_rcv.sh into the kernel selftests.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Suggested-by: Ferenc Fejes <fejes@inf.elte.hu>
Signed-off-by: Anna Emese Nyiri <annaemesenyiri@gmail.com>

---
 tools/testing/selftests/net/Makefile          |   1 +
 tools/testing/selftests/net/so_rcv_listener.c | 147 ++++++++++++++++++
 tools/testing/selftests/net/test_so_rcv.sh    |  56 +++++++
 3 files changed, 204 insertions(+)
 create mode 100644 tools/testing/selftests/net/so_rcv_listener.c
 create mode 100755 tools/testing/selftests/net/test_so_rcv.sh

Comments

Jakub Kicinski Jan. 29, 2025, 8:05 p.m. UTC | #1 
On Wed, 29 Jan 2025 15:36:01 +0100 Anna Emese Nyiri wrote:
> diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> index 73ee88d6b043..98f05473e672 100644
> --- a/tools/testing/selftests/net/Makefile
> +++ b/tools/testing/selftests/net/Makefile
> @@ -33,6 +33,7 @@ TEST_PROGS += gro.sh
>  TEST_PROGS += gre_gso.sh
>  TEST_PROGS += cmsg_so_mark.sh
>  TEST_PROGS += cmsg_so_priority.sh
> +TEST_PROGS += test_so_rcv.sh

You need to add the C part to the TEST_GEN_PROGS, otherwise it won't
get built. We're seeing:

./test_so_rcv.sh: line 25: ./so_rcv_listener: No such file or directory

in the CI.

> +	memset(&recv_addr, 0, sizeof(recv_addr));
> +	recv_addr.sin_family = AF_INET;
> +	recv_addr.sin_port = htons(atoi(opt.service));
> +
> +	if (inet_pton(AF_INET, opt.host, &recv_addr.sin_addr) <= 0) {
> +		perror("Invalid address");
> +		ret_value = -errno;
> +		goto cleanup;
> +	}

Any reason not to use getaddrinfo() ?

Otherwise LGTM, thanks for following up!
Anna Nyiri Jan. 30, 2025, 1:27 p.m. UTC | #2 
Jakub Kicinski <kuba@kernel.org> ezt írta (időpont: 2025. jan. 29., Sze, 21:05):
>
> On Wed, 29 Jan 2025 15:36:01 +0100 Anna Emese Nyiri wrote:
> > diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> > index 73ee88d6b043..98f05473e672 100644
> > --- a/tools/testing/selftests/net/Makefile
> > +++ b/tools/testing/selftests/net/Makefile
> > @@ -33,6 +33,7 @@ TEST_PROGS += gro.sh
> >  TEST_PROGS += gre_gso.sh
> >  TEST_PROGS += cmsg_so_mark.sh
> >  TEST_PROGS += cmsg_so_priority.sh
> > +TEST_PROGS += test_so_rcv.sh
>
> You need to add the C part to the TEST_GEN_PROGS, otherwise it won't
> get built. We're seeing:
>
> ./test_so_rcv.sh: line 25: ./so_rcv_listener: No such file or directory
>
> in the CI.
>
> > +     memset(&recv_addr, 0, sizeof(recv_addr));
> > +     recv_addr.sin_family = AF_INET;
> > +     recv_addr.sin_port = htons(atoi(opt.service));
> > +
> > +     if (inet_pton(AF_INET, opt.host, &recv_addr.sin_addr) <= 0) {
> > +             perror("Invalid address");
> > +             ret_value = -errno;
> > +             goto cleanup;
> > +     }
>
> Any reason not to use getaddrinfo() ?

I chose inet_pton() over getaddrinfo() because getaddrinfo() depends
on libnss, which can cause warnings and linking issues in static
builds. In contrast, inet_pton() is fully part of libc, so it seemed
like a safer choice.

> Otherwise LGTM, thanks for following up!
> --
> pw-bot: cr
Willem de Bruijn Jan. 30, 2025, 3:06 p.m. UTC | #3 
Jakub Kicinski wrote:
> On Wed, 29 Jan 2025 15:36:01 +0100 Anna Emese Nyiri wrote:
> > diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> > index 73ee88d6b043..98f05473e672 100644
> > --- a/tools/testing/selftests/net/Makefile
> > +++ b/tools/testing/selftests/net/Makefile
> > @@ -33,6 +33,7 @@ TEST_PROGS += gro.sh
> >  TEST_PROGS += gre_gso.sh
> >  TEST_PROGS += cmsg_so_mark.sh
> >  TEST_PROGS += cmsg_so_priority.sh
> > +TEST_PROGS += test_so_rcv.sh
> 
> You need to add the C part to the TEST_GEN_PROGS, otherwise it won't
> get built. We're seeing:

and to .gitignore
 
> ./test_so_rcv.sh: line 25: ./so_rcv_listener: No such file or directory
> 
> in the CI.
> 
> > +	memset(&recv_addr, 0, sizeof(recv_addr));
> > +	recv_addr.sin_family = AF_INET;
> > +	recv_addr.sin_port = htons(atoi(opt.service));
> > +
> > +	if (inet_pton(AF_INET, opt.host, &recv_addr.sin_addr) <= 0) {
> > +		perror("Invalid address");
> > +		ret_value = -errno;
> > +		goto cleanup;
> > +	}
> 
> Any reason not to use getaddrinfo() ?
> 
> Otherwise LGTM, thanks for following up!
> -- 
> pw-bot: cr
Willem de Bruijn Jan. 30, 2025, 3:15 p.m. UTC | #4 
Anna Emese Nyiri wrote:
> Introduce tests to verify the correct functionality of the SO_RCVMARK and 
> SO_RCVPRIORITY socket options.
> 
> Key changes include:
> 
> - so_rcv_listener.c: Implements a receiver application to test the correct 
> behavior of the SO_RCVMARK and SO_RCVPRIORITY options.
> - test_so_rcv.sh: Provides a shell script to automate testing for these options.
> - Makefile: Integrates test_so_rcv.sh into the kernel selftests.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Suggested-by: Ferenc Fejes <fejes@inf.elte.hu>
> Signed-off-by: Anna Emese Nyiri <annaemesenyiri@gmail.com>

No need for a cover letter to a single patch.

net-next is closed.

> 
> ---
>  tools/testing/selftests/net/Makefile          |   1 +
>  tools/testing/selftests/net/so_rcv_listener.c | 147 ++++++++++++++++++
>  tools/testing/selftests/net/test_so_rcv.sh    |  56 +++++++
>  3 files changed, 204 insertions(+)
>  create mode 100644 tools/testing/selftests/net/so_rcv_listener.c
>  create mode 100755 tools/testing/selftests/net/test_so_rcv.sh
> 
> diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> index 73ee88d6b043..98f05473e672 100644
> --- a/tools/testing/selftests/net/Makefile
> +++ b/tools/testing/selftests/net/Makefile
> @@ -33,6 +33,7 @@ TEST_PROGS += gro.sh
>  TEST_PROGS += gre_gso.sh
>  TEST_PROGS += cmsg_so_mark.sh
>  TEST_PROGS += cmsg_so_priority.sh
> +TEST_PROGS += test_so_rcv.sh
>  TEST_PROGS += cmsg_time.sh cmsg_ipv6.sh
>  TEST_PROGS += netns-name.sh
>  TEST_PROGS += nl_netdev.py
> diff --git a/tools/testing/selftests/net/so_rcv_listener.c b/tools/testing/selftests/net/so_rcv_listener.c
> new file mode 100644
> index 000000000000..53b09582a7e3
> --- /dev/null
> +++ b/tools/testing/selftests/net/so_rcv_listener.c
> @@ -0,0 +1,147 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <errno.h>
> +#include <netdb.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <linux/types.h>
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +#include <arpa/inet.h>
> +
> +#ifndef SO_RCVPRIORITY
> +#define SO_RCVPRIORITY 82
> +#endif
> +
> +struct options {
> +	__u32 val;
> +	int name;
> +	int rcvname;
> +	const char *host;
> +	const char *service;
> +} opt;
> +
> +static void __attribute__((noreturn)) usage(const char *bin)
> +{
> +	printf("Usage: %s [opts] <dst host> <dst port / service>\n", bin);
> +	printf("Options:\n"
> +		"\t\t-M val  Test SO_RCVMARK\n"
> +		"\t\t-P val  Test SO_RCVPRIORITY\n"
> +		"");
> +	exit(EXIT_FAILURE);
> +}
> +
> +static void parse_args(int argc, char *argv[])
> +{
> +	int o;
> +
> +	while ((o = getopt(argc, argv, "M:P:")) != -1) {
> +		switch (o) {
> +		case 'M':
> +			opt.val = atoi(optarg);
> +			opt.name = SO_MARK;
> +			opt.rcvname = SO_RCVMARK;
> +			break;
> +		case 'P':
> +			opt.val = atoi(optarg);
> +			opt.name = SO_PRIORITY;
> +			opt.rcvname = SO_RCVPRIORITY;
> +			break;
> +		default:
> +			usage(argv[0]);
> +			break;
> +		}
> +	}
> +
> +	if (optind != argc - 2)
> +		usage(argv[0]);
> +
> +	opt.host = argv[optind];
> +	opt.service = argv[optind + 1];
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	int err = 0;
> +	int recv_fd = -1;
> +	int ret_value = 0;
> +	__u32 recv_val;
> +	struct cmsghdr *cmsg;
> +	char cbuf[1024];

Please use CMSG_SPACE and anticipate the minimum required space needed.

> +	char recv_buf[1024];
> +	struct iovec iov[1];
> +	struct msghdr msg;
> +	struct sockaddr_in recv_addr;

In general code should not be IPv4 only. In this case the logic in the
kernel is the same for IPv4 and IPv6, so I guess it's okay. If
explicitly testing only one of the two, I would suggest IPv6.

> +
> +	parse_args(argc, argv);
> +
> +	recv_fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
> +	if (recv_fd < 0) {
> +		perror("Can't open recv socket");
> +		ret_value = -errno;
> +		goto cleanup;
> +	}
> +
> +	err = setsockopt(recv_fd, SOL_SOCKET, opt.rcvname, &opt.val, sizeof(opt.val));
> +	if (err < 0) {
> +		perror("Recv setsockopt error");
> +		ret_value = -errno;
> +		goto cleanup;
> +	}
> +
> +	memset(&recv_addr, 0, sizeof(recv_addr));
> +	recv_addr.sin_family = AF_INET;
> +	recv_addr.sin_port = htons(atoi(opt.service));
> +
> +	if (inet_pton(AF_INET, opt.host, &recv_addr.sin_addr) <= 0) {
> +		perror("Invalid address");
> +		ret_value = -errno;
> +		goto cleanup;
> +	}
> +
> +	err = bind(recv_fd, (struct sockaddr *)&recv_addr, sizeof(recv_addr));
> +	if (err < 0) {
> +		perror("Recv bind error");
> +		ret_value = -errno;
> +		goto cleanup;
> +	}
> +
> +	iov[0].iov_base = recv_buf;
> +	iov[0].iov_len = sizeof(recv_buf);
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.msg_iov = iov;
> +	msg.msg_iovlen = 1;
> +	msg.msg_control = cbuf;
> +	msg.msg_controllen = sizeof(cbuf);
> +
> +	err = recvmsg(recv_fd, &msg, 0);
> +	if (err < 0) {
> +		perror("Message receive error");
> +		ret_value = -errno;
> +		goto cleanup;
> +	}
> +
> +	for (cmsg = CMSG_FIRSTHDR(&msg); cmsg != NULL; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
> +		if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == opt.name) {
> +			recv_val = *(__u32 *)CMSG_DATA(cmsg);
> +			printf("Received value: %u\n", recv_val);
> +
> +			if (recv_val != opt.val) {
> +				fprintf(stderr, "Error: expected value: %u, got: %u\n",
> +					opt.val, recv_val);
> +				ret_value = -EINVAL;
> +				goto cleanup;
> +			}
> +		}
> +	}
> +
> +cleanup:
> +	if (recv_fd >= 0)
> +		close(recv_fd);
> +
> +	return ret_value;
> +}
> diff --git a/tools/testing/selftests/net/test_so_rcv.sh b/tools/testing/selftests/net/test_so_rcv.sh
> new file mode 100755
> index 000000000000..12d37f9ab905
> --- /dev/null
> +++ b/tools/testing/selftests/net/test_so_rcv.sh
> @@ -0,0 +1,56 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +HOST=127.0.0.1
> +PORT=1234
> +TOTAL_TESTS=0
> +FAILED_TESTS=0
> +
> +declare -A TESTS=(
> +	["SO_RCVPRIORITY"]="-P 2"
> +	["SO_RCVMARK"]="-M 3"
> +)
> +
> +check_result() {
> +	((TOTAL_TESTS++))
> +	if [ "$1" -ne 0 ]; then
> +		((FAILED_TESTS++))
> +	fi
> +}
> +
> +for test_name in "${!TESTS[@]}"; do
> +	echo "Running $test_name test"
> +	arg=${TESTS[$test_name]}
> +
> +	./so_rcv_listener $arg $HOST $PORT &
> +	LISTENER_PID=$!
> +
> +	if ./cmsg_sender $arg $HOST $PORT; then
> +		echo "Sender succeeded for $test_name"

nit: such verbose comments are not very informative.

> +	else
> +		echo "Sender failed for $test_name"
> +		kill "$LISTENER_PID" 2>/dev/null
> +		wait "$LISTENER_PID"
> +		check_result 1
> +		continue
> +	fi
> +
> +	wait "$LISTENER_PID"
> +	LISTENER_EXIT_CODE=$?
> +
> +	if [ "$LISTENER_EXIT_CODE" -eq 0 ]; then
> +		echo "Rcv test OK for $test_name"
> +		check_result 0
> +	else
> +		echo "Rcv test FAILED for $test_name"
> +		check_result 1
> +	fi
> +done
> +
> +if [ "$FAILED_TESTS" -ne 0 ]; then
> +	echo "FAIL - $FAILED_TESTS/$TOTAL_TESTS tests failed"
> +	exit 1

please use kselftest exit codes: KSFT_FAIL

> +else
> +	echo "OK - All $TOTAL_TESTS tests passed"
> +	exit 0

KSFT_PASS

> +fi
> -- 
> 2.43.0
>
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 73ee88d6b043..98f05473e672 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -33,6 +33,7 @@  TEST_PROGS += gro.sh
 TEST_PROGS += gre_gso.sh
 TEST_PROGS += cmsg_so_mark.sh
 TEST_PROGS += cmsg_so_priority.sh
+TEST_PROGS += test_so_rcv.sh
 TEST_PROGS += cmsg_time.sh cmsg_ipv6.sh
 TEST_PROGS += netns-name.sh
 TEST_PROGS += nl_netdev.py
diff --git a/tools/testing/selftests/net/so_rcv_listener.c b/tools/testing/selftests/net/so_rcv_listener.c
new file mode 100644
index 000000000000..53b09582a7e3
--- /dev/null
+++ b/tools/testing/selftests/net/so_rcv_listener.c
@@ -0,0 +1,147 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <errno.h>
+#include <netdb.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <linux/types.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
+
+#ifndef SO_RCVPRIORITY
+#define SO_RCVPRIORITY 82
+#endif
+
+struct options {
+	__u32 val;
+	int name;
+	int rcvname;
+	const char *host;
+	const char *service;
+} opt;
+
+static void __attribute__((noreturn)) usage(const char *bin)
+{
+	printf("Usage: %s [opts] <dst host> <dst port / service>\n", bin);
+	printf("Options:\n"
+		"\t\t-M val  Test SO_RCVMARK\n"
+		"\t\t-P val  Test SO_RCVPRIORITY\n"
+		"");
+	exit(EXIT_FAILURE);
+}
+
+static void parse_args(int argc, char *argv[])
+{
+	int o;
+
+	while ((o = getopt(argc, argv, "M:P:")) != -1) {
+		switch (o) {
+		case 'M':
+			opt.val = atoi(optarg);
+			opt.name = SO_MARK;
+			opt.rcvname = SO_RCVMARK;
+			break;
+		case 'P':
+			opt.val = atoi(optarg);
+			opt.name = SO_PRIORITY;
+			opt.rcvname = SO_RCVPRIORITY;
+			break;
+		default:
+			usage(argv[0]);
+			break;
+		}
+	}
+
+	if (optind != argc - 2)
+		usage(argv[0]);
+
+	opt.host = argv[optind];
+	opt.service = argv[optind + 1];
+}
+
+int main(int argc, char *argv[])
+{
+	int err = 0;
+	int recv_fd = -1;
+	int ret_value = 0;
+	__u32 recv_val;
+	struct cmsghdr *cmsg;
+	char cbuf[1024];
+	char recv_buf[1024];
+	struct iovec iov[1];
+	struct msghdr msg;
+	struct sockaddr_in recv_addr;
+
+	parse_args(argc, argv);
+
+	recv_fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
+	if (recv_fd < 0) {
+		perror("Can't open recv socket");
+		ret_value = -errno;
+		goto cleanup;
+	}
+
+	err = setsockopt(recv_fd, SOL_SOCKET, opt.rcvname, &opt.val, sizeof(opt.val));
+	if (err < 0) {
+		perror("Recv setsockopt error");
+		ret_value = -errno;
+		goto cleanup;
+	}
+
+	memset(&recv_addr, 0, sizeof(recv_addr));
+	recv_addr.sin_family = AF_INET;
+	recv_addr.sin_port = htons(atoi(opt.service));
+
+	if (inet_pton(AF_INET, opt.host, &recv_addr.sin_addr) <= 0) {
+		perror("Invalid address");
+		ret_value = -errno;
+		goto cleanup;
+	}
+
+	err = bind(recv_fd, (struct sockaddr *)&recv_addr, sizeof(recv_addr));
+	if (err < 0) {
+		perror("Recv bind error");
+		ret_value = -errno;
+		goto cleanup;
+	}
+
+	iov[0].iov_base = recv_buf;
+	iov[0].iov_len = sizeof(recv_buf);
+
+	memset(&msg, 0, sizeof(msg));
+	msg.msg_iov = iov;
+	msg.msg_iovlen = 1;
+	msg.msg_control = cbuf;
+	msg.msg_controllen = sizeof(cbuf);
+
+	err = recvmsg(recv_fd, &msg, 0);
+	if (err < 0) {
+		perror("Message receive error");
+		ret_value = -errno;
+		goto cleanup;
+	}
+
+	for (cmsg = CMSG_FIRSTHDR(&msg); cmsg != NULL; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
+		if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == opt.name) {
+			recv_val = *(__u32 *)CMSG_DATA(cmsg);
+			printf("Received value: %u\n", recv_val);
+
+			if (recv_val != opt.val) {
+				fprintf(stderr, "Error: expected value: %u, got: %u\n",
+					opt.val, recv_val);
+				ret_value = -EINVAL;
+				goto cleanup;
+			}
+		}
+	}
+
+cleanup:
+	if (recv_fd >= 0)
+		close(recv_fd);
+
+	return ret_value;
+}
diff --git a/tools/testing/selftests/net/test_so_rcv.sh b/tools/testing/selftests/net/test_so_rcv.sh
new file mode 100755
index 000000000000..12d37f9ab905
--- /dev/null
+++ b/tools/testing/selftests/net/test_so_rcv.sh
@@ -0,0 +1,56 @@ 
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+HOST=127.0.0.1
+PORT=1234
+TOTAL_TESTS=0
+FAILED_TESTS=0
+
+declare -A TESTS=(
+	["SO_RCVPRIORITY"]="-P 2"
+	["SO_RCVMARK"]="-M 3"
+)
+
+check_result() {
+	((TOTAL_TESTS++))
+	if [ "$1" -ne 0 ]; then
+		((FAILED_TESTS++))
+	fi
+}
+
+for test_name in "${!TESTS[@]}"; do
+	echo "Running $test_name test"
+	arg=${TESTS[$test_name]}
+
+	./so_rcv_listener $arg $HOST $PORT &
+	LISTENER_PID=$!
+
+	if ./cmsg_sender $arg $HOST $PORT; then
+		echo "Sender succeeded for $test_name"
+	else
+		echo "Sender failed for $test_name"
+		kill "$LISTENER_PID" 2>/dev/null
+		wait "$LISTENER_PID"
+		check_result 1
+		continue
+	fi
+
+	wait "$LISTENER_PID"
+	LISTENER_EXIT_CODE=$?
+
+	if [ "$LISTENER_EXIT_CODE" -eq 0 ]; then
+		echo "Rcv test OK for $test_name"
+		check_result 0
+	else
+		echo "Rcv test FAILED for $test_name"
+		check_result 1
+	fi
+done
+
+if [ "$FAILED_TESTS" -ne 0 ]; then
+	echo "FAIL - $FAILED_TESTS/$TOTAL_TESTS tests failed"
+	exit 1
+else
+	echo "OK - All $TOTAL_TESTS tests passed"
+	exit 0
+fi