Message-ID: <aecfafb8-f556-4d7e-941d-2975f3f30396@rbox.co>
Date: Thu, 12 Dec 2024 19:26:39 +0100
From: Michal Luczaj <mhal@...x.co>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next 4/4] vsock/test: Add test for MSG_ZEROCOPY
 completion memory leak

On 12/10/24 16:36, Stefano Garzarella wrote:
> On Fri, Dec 06, 2024 at 07:34:54PM +0100, Michal Luczaj wrote:
>> [...]
>> +static void test_stream_msgzcopy_leak_zcskb_client(const struct test_opts *opts)
>> +{
>> +	char buf[1024] = { 0 };
>> +	ssize_t optmem_max;
>> +	int fd, res;
>> +	FILE *f;
>> +
>> +	f = fopen("/proc/sys/net/core/optmem_max", "r");
>> +	if (!f) {
>> +		perror("fopen(optmem_max)");
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	if (fscanf(f, "%zd", &optmem_max) != 1 || optmem_max > ~0U / 2) {
>> +		fprintf(stderr, "fscanf(optmem_max) failed\n");
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	fclose(f);
>> +
>> +	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>> +	if (fd < 0) {
>> +		perror("connect");
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	enable_so_zerocopy_check(fd);
>> +
>> +	/* The idea is to fail virtio_transport_init_zcopy_skb() by hitting
>> +	 * core.sysctl_optmem_max (sysctl net.core.optmem_max) limit check in
>> +	 * sock_omalloc().
>> +	 */
>> +	optmem_max *= 2;
>> +	errno = 0;
>> +	do {
>> +		res = send(fd, buf, sizeof(buf), MSG_ZEROCOPY);
>> +		optmem_max -= res;
>> +	} while (res > 0 && optmem_max > 0);
>> +
>> +	if (errno != ENOMEM) {
>> +		fprintf(stderr, "expected ENOMEM on send()\n");
> 
> This test is failing in my suite with this message (I added errno
> in the message that maybe we can add to understand better what we
> expect, and what we saw) [...]

Thanks, can reproduce. And I'm surprised it ever worked at all. What I did
is completely and utterly wrong :)

The idea was to exhaust sk_omem_alloc by growing some skbs. First thing
I've missed: no matter the size of the buffer being send(), sock_omalloc()
is always requested with size=0[*]. Sure, skb->truesize is non-zero, so it
does bump the counter, but it won't work as intended.

Then comes vhost transport: sk_omem_alloc won't stay incremented, because
skbs are processed and consumed. That's a race between transport and us.

So here's another desperate attempt to do it without elevated privileges:
exhaust sk_omem_alloc by abusing sendmsg()'s `sock_kmalloc(sock->sk,
ctl_len, GFP_KERNEL)`. Code below, tested under what I hope resembles your
nested setup, i.e.

L1$ --mode=server --peer-cid=4
L2$ --mode=client --peer-cid=2

L1$ --mode=client --peer-cid=4
L2$ --mode=server --peer-cid=2

Please let me know if it works for you.

That said, I really think this test should be scrapped. I'm afraid it will
break sooner or later. And since kmemleak needs root anyway, perhaps it's
better to use failslab fault injection for this?

[*] It's the only caller. Should @size be dropped from sock_omalloc()?

Sorry for the mess,
Michal

>From 52cd25e49649f853e75ef82c90ff360ee0a54a50 Mon Sep 17 00:00:00 2001
From: Michal Luczaj <mhal@...x.co>
Date: Wed, 4 Dec 2024 02:38:16 +0100
Subject: [PATCH] vsock/test: Add test for MSG_ZEROCOPY completion memory leak

Exercise the ENOMEM error path by attempting to hit net.core.optmem_max
limit on send().

Fixed by commit 60cf6206a1f5 ("virtio/vsock: Improve MSG_ZEROCOPY error
handling").

Signed-off-by: Michal Luczaj <mhal@...x.co>
---
 tools/testing/vsock/vsock_test.c | 82 ++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 0a930803227a..d64e681f2904 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -1552,6 +1552,83 @@ static void test_stream_msgzcopy_leak_errq_server(const struct test_opts *opts)
 	close(fd);
 }
 
+static void test_stream_msgzcopy_leak_zcskb_client(const struct test_opts *opts)
+{
+	size_t optmem_max, chunk_size;
+	struct msghdr msg = { 0 };
+	struct iovec iov = { 0 };
+	char *chunk, buf = 'x';
+	int fd, res;
+	FILE *f;
+
+	f = fopen("/proc/sys/net/core/optmem_max", "r");
+	if (!f) {
+		perror("fopen(optmem_max)");
+		exit(EXIT_FAILURE);
+	}
+
+	if (fscanf(f, "%zu", &optmem_max) != 1) {
+		fprintf(stderr, "fscanf(optmem_max) failed\n");
+		exit(EXIT_FAILURE);
+	}
+
+	fclose(f);
+
+	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
+	if (fd < 0) {
+		perror("connect");
+		exit(EXIT_FAILURE);
+	}
+
+	enable_so_zerocopy_check(fd);
+
+	/* The idea is to fail virtio_transport_init_zcopy_skb() by hitting
+	 * core.sysctl_optmem_max (sysctl net.core.optmem_max) limit check in
+	 * sock_omalloc(sk, 0, GFP_KERNEL). Stuff sk->sk_omem_alloc with cmsg,
+	 * see net/socket.c:____sys_sendmsg().
+	 */
+
+	chunk_size = CMSG_SPACE(optmem_max - 1);
+	chunk = malloc(chunk_size);
+	if (!chunk) {
+		perror("malloc");
+		exit(EXIT_FAILURE);
+	}
+	memset(chunk, 0, chunk_size);
+
+	iov.iov_base = &buf;
+	iov.iov_len = 1;
+
+	msg.msg_iov = &iov;
+	msg.msg_iovlen = 1;
+	msg.msg_control = chunk;
+	msg.msg_controllen = optmem_max - 1;
+
+	errno = 0;
+	res = sendmsg(fd, &msg, MSG_ZEROCOPY);
+	if (res >= 0 || errno != ENOMEM) {
+		fprintf(stderr, "expected ENOMEM, got errno=%d res=%d\n",
+			errno, res);
+		exit(EXIT_FAILURE);
+	}
+
+	close(fd);
+}
+
+static void test_stream_msgzcopy_leak_zcskb_server(const struct test_opts *opts)
+{
+	int fd;
+
+	fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
+	if (fd < 0) {
+		perror("accept");
+		exit(EXIT_FAILURE);
+	}
+
+	vsock_wait_remote_close(fd);
+	close(fd);
+}
+
 static struct test_case test_cases[] = {
 	{
 		.name = "SOCK_STREAM connection reset",
@@ -1692,6 +1769,11 @@ static struct test_case test_cases[] = {
 		.run_client = test_stream_msgzcopy_leak_errq_client,
 		.run_server = test_stream_msgzcopy_leak_errq_server,
 	},
+	{
+		.name = "SOCK_STREAM MSG_ZEROCOPY leak completion skb",
+		.run_client = test_stream_msgzcopy_leak_zcskb_client,
+		.run_server = test_stream_msgzcopy_leak_zcskb_server,
+	},
 	{},
 };
 
-- 
2.47.1

Powered by blists - more mailing lists