netdev - Re: [PATCH net-next v2 4/6] vsock/test: Add test for accept

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bl3osg7ze6bjivu53j5vdlrtkzq35vk3zbp2veosyklp53rf2i@drb2efczau6n>
Date: Mon, 16 Dec 2024 15:35:24 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Michal Luczaj <mhal@...x.co>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 4/6] vsock/test: Add test for accept_queue
 memory leak

On Mon, Dec 16, 2024 at 01:01:00PM +0100, Michal Luczaj wrote:
>Attempt to enqueue a child after the queue was flushed, but before
>SOCK_DONE flag has been set.
>
>Test tries to produce a memory leak, kmemleak should be employed. Dealing
>with a race condition, test by its very nature may lead to a false
>negative.
>
>Fixed by commit d7b0ff5a8667 ("virtio/vsock: Fix accept_queue memory
>leak").
>
>Signed-off-by: Michal Luczaj <mhal@...x.co>
>---
> tools/testing/vsock/vsock_test.c | 51 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 1ad1fbba10307c515e31816a2529befd547f7fd7..1a9bd81758675a0f2b9b6b0ad9271c45f89a4860 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -1474,6 +1474,52 @@ static void test_stream_cred_upd_on_set_rcvlowat(const struct test_opts *opts)
> 	test_stream_credit_update_test(opts, false);
> }
>
>+/* The goal of test leak_acceptq is to stress the race between connect() and
>+ * close(listener). Implementation of client/server loops boils down to:
>+ *
>+ * client                server
>+ * ------                ------
>+ * write(CONTINUE)
>+ *                       expect(CONTINUE)
>+ *                       listen()
>+ *                       write(LISTENING)
>+ * expect(LISTENING)
>+ * connect()             close()
>+ */
>+#define ACCEPTQ_LEAK_RACE_TIMEOUT 2 /* seconds */
>+
>+#define CONTINUE	1
>+#define DONE		0

I would add a prefix here, looking at the timeout, I would say
ACCEPTQ_LEAK_RACE_CONTINUE and ACCEPTQ_LEAK_RACE_DONE.

The rest LKGT!
Stefano

>+
>+static void test_stream_leak_acceptq_client(const struct test_opts *opts)
>+{
>+	time_t tout;
>+	int fd;
>+
>+	tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC;
>+	do {
>+		control_writeulong(CONTINUE);
>+
>+		fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
>+		if (fd >= 0)
>+			close(fd);
>+	} while (current_nsec() < tout);
>+
>+	control_writeulong(DONE);
>+}
>+
>+/* Test for a memory leak. User is expected to run kmemleak scan, see README. */
>+static void test_stream_leak_acceptq_server(const struct test_opts *opts)
>+{
>+	int fd;
>+
>+	while (control_readulong() == CONTINUE) {
>+		fd = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port);
>+		control_writeln("LISTENING");
>+		close(fd);
>+	}
>+}
>+
> static struct test_case test_cases[] = {
> 	{
> 		.name = "SOCK_STREAM connection reset",
>@@ -1604,6 +1650,11 @@ static struct test_case test_cases[] = {
> 		.run_client = test_seqpacket_unsent_bytes_client,
> 		.run_server = test_seqpacket_unsent_bytes_server,
> 	},
>+	{
>+		.name = "SOCK_STREAM leak accept queue",
>+		.run_client = test_stream_leak_acceptq_client,
>+		.run_server = test_stream_leak_acceptq_server,
>+	},
> 	{},
> };
>
>
>-- 
>2.47.1
>