Message-ID: <fyuxxdc4rgop73kkevef5gfujgac3f3we5mii4bpoznonlr3nn@6mjpi5df3dlg>
Date: Thu, 18 Dec 2025 10:45:23 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Melbin K Mathew <mlbnkm1@...il.com>
Cc: stefanha@...hat.com, kvm@...r.kernel.org, netdev@...r.kernel.org, 
	virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org, mst@...hat.com, 
	jasowang@...hat.com, xuanzhuo@...ux.alibaba.com, eperezma@...hat.com, 
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	horms@...nel.org
Subject: Re: [PATCH net v4 4/4] vsock/test: add stream TX credit bounds test

On Wed, Dec 17, 2025 at 07:12:06PM +0100, Melbin K Mathew wrote:
>Add a regression test for the TX credit bounds fix. The test verifies
>that a sender with a small local buffer size cannot queue excessive
>data even when the peer advertises a large receive buffer.
>
>The client:
>  - Sets a small buffer size (64 KiB)
>  - Connects to server (which advertises 2 MiB buffer)
>  - Sends in non-blocking mode until EAGAIN
>  - Verifies total queued data is bounded
>
>This guards against the original vulnerability where a remote peer
>could cause unbounded kernel memory allocation by advertising a large
>buffer and reading slowly.
>
>Suggested-by: Stefano Garzarella <sgarzare@...hat.com>
>Signed-off-by: Melbin K Mathew <mlbnkm1@...il.com>
>---
> tools/testing/vsock/vsock_test.c | 103 +++++++++++++++++++++++++++++++
> 1 file changed, 103 insertions(+)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 0e8e173dfbdc..9f4598ee45f9 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -347,6 +347,7 @@ static void test_stream_msg_peek_server(const struct test_opts *opts)
> }
>
> #define SOCK_BUF_SIZE (2 * 1024 * 1024)
>+#define SMALL_SOCK_BUF_SIZE (64 * 1024ULL)
> #define MAX_MSG_PAGES 4
>
> static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>@@ -2203,6 +2204,103 @@ static void test_stream_nolinger_server(const struct test_opts *opts)
> 	close(fd);
> }
>
>+static void test_stream_tx_credit_bounds_client(const struct test_opts *opts)
>+{
>+	unsigned long long sock_buf_size;
>+	char buf[4096];
>+	size_t total = 0;
>+	ssize_t sent;
>+	int fd;
>+	int flags;
>+
>+	memset(buf, 'A', sizeof(buf));
>+
>+	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>+	if (fd < 0) {
>+		perror("connect");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	sock_buf_size = SMALL_SOCK_BUF_SIZE;
>+
>+	setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>+			     sock_buf_size,
>+			     "setsockopt(SO_VM_SOCKETS_BUFFER_MAX_SIZE)");
>+
>+	setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>+			     sock_buf_size,
>+			     "setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
>+
>+	flags = fcntl(fd, F_GETFL);
>+	if (flags < 0) {
>+		perror("fcntl(F_GETFL)");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (fcntl(fd, F_SETFL, flags | O_NONBLOCK) < 0) {
>+		perror("fcntl(F_SETFL)");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	control_expectln("SRVREADY");
>+
>+	for (;;) {
>+		sent = send(fd, buf, sizeof(buf), 0);
>+		if (sent > 0) {
>+			total += sent;
>+			continue;
>+		}

This is confusing IMO, also perror() when `sent == 0` is not right.
What about this:

		sent = send(fd, buf, sizeof(buf), 0);
		if (sent == 0) {
			fprintf(stderr, "unexpected EOF while sending bytes\n");
			exit(EXIT_FAILURE);
		}

		if (sent < 0) {
			if (errno == EAGAIN || errno == EWOULDBLOCK)
				break

			perror("send");
			exit(EXIT_FAILURE);
		}

		total += sent;

>+		if (sent < 0 && (errno == EAGAIN || errno == EWOULDBLOCK))
>+			break;
>+
>+		perror("send");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	/*
>+	 * With TX credit bounded by local buffer size, sending should
>+	 * stall quickly. Allow some overhead but fail if we queued an
>+	 * unreasonable amount.
>+	 */
>+	if (total > (size_t)(SMALL_SOCK_BUF_SIZE * 4)) {

Why "* 4" ?

>+		fprintf(stderr,
>+			"TX credit too large: queued %zu bytes (expected <= %llu)\n",
>+			total, (unsigned long long)(SMALL_SOCK_BUF_SIZE * 4));
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	control_writeln("CLIDONE");

This can be moved after the for loop.

>+	close(fd);
>+}
>+
>+static void test_stream_tx_credit_bounds_server(const struct test_opts *opts)
>+{
>+	unsigned long long sock_buf_size;
>+	int fd;
>+
>+	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
>+	if (fd < 0) {
>+		perror("accept");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	/* Server advertises large buffer; client should still be bounded */
>+	sock_buf_size = SOCK_BUF_SIZE;
>+
>+	setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>+			     sock_buf_size,
>+			     "setsockopt(SO_VM_SOCKETS_BUFFER_MAX_SIZE)");
>+
>+	setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>+			     sock_buf_size,
>+			     "setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
>+
>+	control_writeln("SRVREADY");
>+	control_expectln("CLIDONE");
>+
>+	close(fd);
>+}
>+
> static struct test_case test_cases[] = {
> 	{
> 		.name = "SOCK_STREAM connection reset",
>@@ -2382,6 +2480,11 @@ static struct test_case test_cases[] = {
> 		.run_client = test_seqpacket_unread_bytes_client,
> 		.run_server = test_seqpacket_unread_bytes_server,
> 	},
>+	{
>+		.name = "SOCK_STREAM TX credit bounds",
>+		.run_client = test_stream_tx_credit_bounds_client,
>+		.run_server = test_stream_tx_credit_bounds_server,
>+	},
> 	{},
> };
>
>-- 
>2.34.1
>

Powered by blists - more mailing lists