Re: [PATCH net-next 2/4] vsock/test: Add test for accept_queue memory leak

On Thu, Dec 12, 2024 at 11:12:19PM +0100, Michal Luczaj wrote:

On 12/10/24 17:18, Stefano Garzarella wrote:

quoted

On Fri, Dec 06, 2024 at 07:34:52PM +0100, Michal Luczaj wrote:

quoted

[...]
+#define ACCEPTQ_LEAK_RACE_TIMEOUT 2 /* seconds */
+
+static void test_stream_leak_acceptq_client(const struct test_opts *opts)
+{
+	struct sockaddr_vm addr = {
+		.svm_family = AF_VSOCK,
+		.svm_port = opts->peer_port,
+		.svm_cid = opts->peer_cid
+	};
+	time_t tout;
+	int fd;
+
+	tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC;
+	do {
+		control_writeulong(1);

Can we use control_writeln() and control_expectln()?

Please see below.

quoted

quoted

+
+		fd = socket(AF_VSOCK, SOCK_STREAM, 0);
+		if (fd < 0) {
+			perror("socket");
+			exit(EXIT_FAILURE);
+		}
+

Do we need another control messages (server -> client) here to be sure
the server is listening?

Ahh, I get your point.

quoted

quoted

+		connect(fd, (struct sockaddr *)&addr, sizeof(addr));

What about using `vsock_stream_connect` so you can remove a lot of
code from this function (e.g. sockaddr_vm, socket(), etc.)

We only need to add `control_expectln("LISTENING")` in the server which
should also fix my previous comment.

Sure, I followed your suggestion with

tout = current_nsec() + ACCEPTQ_LEAK_RACE_TIMEOUT * NSEC_PER_SEC;
do {
	control_writeulong(RACE_CONTINUE);
	fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
	if (fd >= 0)
		close(fd);

I'd do
		if (fd < 0) {
			perror("connect");
			exit(EXIT_FAILURE);
		}
		close(fd);

apart of that LGTM!

} while (current_nsec() < tout);
control_writeulong(RACE_DONE);

vs.

while (control_readulong() == RACE_CONTINUE) {
	fd = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port);
	control_writeln("LISTENING");
	close(fd);
}

and it works just fine.

quoted

quoted

+static void test_stream_leak_acceptq_server(const struct test_opts *opts)
+{
+	int fd;
+
+	while (control_readulong()) {

Ah I see, the loop is easier by sending a number.
I would just add some comments when we send 1 and 0 to explain it.

How about the #defines above?

yeah, I like it!

Thanks,
Stefano

Thanks!
Michal