[PATCH net-next] vsock/test: fix test for null ptr deref when transport changes

[PATCH net-next] vsock/test: fix test for null ptr deref when transport changes

[Stefano Garzarella]

From: Stefano Garzarella <sgarzare@redhat.com>

In test_stream_transport_change_client(), the client sends CONTROL_CONTINUE
on each iteration, even when connect() is unsuccessful. This causes a flood
of control messages in the server that hangs around for more than 10
seconds after the test finishes, triggering several timeouts and causing
subsequent tests to fail. This was discovered in testing a newly proposed
test that failed in this way on the client side:
    ...
    33 - SOCK_STREAM transport change null-ptr-deref...ok
    34 - SOCK_STREAM ioctl(SIOCINQ) functionality...recv timed out

The CONTROL_CONTINUE message is used only to tell to the server to call
accept() to consume successful connections, so that subsequent connect()
will not fail for finding the queue full.

Send CONTROL_CONTINUE message only when the connect() has succeeded, or
found the queue full. Note that the second connect() can also succeed if
the first one was interrupted after sending the request.

Fixes: 3a764d93385c ("vsock/test: Add test for null ptr deref when transport changes")
Cc: leonardi@redhat.com
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 tools/testing/vsock/vsock_test.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index be6ce764f694..630110ee31df 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -1937,6 +1937,7 @@ static void test_stream_transport_change_client(const struct test_opts *opts)
 			.svm_cid = opts->peer_cid,
 			.svm_port = opts->peer_port,
 		};
+		bool send_control = false;
 		int s;
 
 		s = socket(AF_VSOCK, SOCK_STREAM, 0);
@@ -1957,19 +1958,29 @@ static void test_stream_transport_change_client(const struct test_opts *opts)
 			exit(EXIT_FAILURE);
 		}
 
+		/* Notify the server if the connect() is successful or the
+		 * receiver connection queue is full, so it will do accept()
+		 * to drain it.
+		 */
+		if (!ret || errno == ECONNRESET)
+			send_control = true;
+
 		/* Set CID to 0 cause a transport change. */
 		sa.svm_cid = 0;
 
-		/* Ignore return value since it can fail or not.
-		 * If the previous connect is interrupted while the
-		 * connection request is already sent, the second
+		/* There is a case where this will not fail:
+		 * if the previous connect() is interrupted while the
+		 * connection request is already sent, this second
 		 * connect() will wait for the response.
 		 */
-		connect(s, (struct sockaddr *)&sa, sizeof(sa));
+		ret = connect(s, (struct sockaddr *)&sa, sizeof(sa));
+		if (!ret || errno == ECONNRESET)
+			send_control = true;
 
 		close(s);
 
-		control_writeulong(CONTROL_CONTINUE);
+		if (send_control)
+			control_writeulong(CONTROL_CONTINUE);
 
 	} while (current_nsec() < tout);
 
-- 
2.50.0

Re: [PATCH net-next] vsock/test: fix test for null ptr deref when transport changes

[Luigi Leonardi]

On Tue, Jul 08, 2025 at 01:17:01PM +0200, Stefano Garzarella wrote:
>From: Stefano Garzarella <sgarzare@redhat.com>
>
>In test_stream_transport_change_client(), the client sends CONTROL_CONTINUE
>on each iteration, even when connect() is unsuccessful. This causes a flood
>of control messages in the server that hangs around for more than 10
>seconds after the test finishes, triggering several timeouts and causing
>subsequent tests to fail. This was discovered in testing a newly proposed
>test that failed in this way on the client side:
>    ...
>    33 - SOCK_STREAM transport change null-ptr-deref...ok
>    34 - SOCK_STREAM ioctl(SIOCINQ) functionality...recv timed out
>
>The CONTROL_CONTINUE message is used only to tell to the server to call
>accept() to consume successful connections, so that subsequent connect()
>will not fail for finding the queue full.
>
>Send CONTROL_CONTINUE message only when the connect() has succeeded, or
>found the queue full. Note that the second connect() can also succeed if
>the first one was interrupted after sending the request.
>
>Fixes: 3a764d93385c ("vsock/test: Add test for null ptr deref when transport changes")
>Cc: leonardi@redhat.com
>Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>---
> tools/testing/vsock/vsock_test.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index be6ce764f694..630110ee31df 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -1937,6 +1937,7 @@ static void test_stream_transport_change_client(const struct test_opts *opts)
> 			.svm_cid = opts->peer_cid,
> 			.svm_port = opts->peer_port,
> 		};
>+		bool send_control = false;
> 		int s;
>
> 		s = socket(AF_VSOCK, SOCK_STREAM, 0);
>@@ -1957,19 +1958,29 @@ static void test_stream_transport_change_client(const struct test_opts *opts)
> 			exit(EXIT_FAILURE);
> 		}
>
>+		/* Notify the server if the connect() is successful or the
>+		 * receiver connection queue is full, so it will do accept()
>+		 * to drain it.
>+		 */
>+		if (!ret || errno == ECONNRESET)
>+			send_control = true;
>+
> 		/* Set CID to 0 cause a transport change. */
> 		sa.svm_cid = 0;
>
>-		/* Ignore return value since it can fail or not.
>-		 * If the previous connect is interrupted while the
>-		 * connection request is already sent, the second
>+		/* There is a case where this will not fail:
>+		 * if the previous connect() is interrupted while the
>+		 * connection request is already sent, this second
> 		 * connect() will wait for the response.
> 		 */
>-		connect(s, (struct sockaddr *)&sa, sizeof(sa));
>+		ret = connect(s, (struct sockaddr *)&sa, sizeof(sa));
>+		if (!ret || errno == ECONNRESET)
>+			send_control = true;
>
> 		close(s);
>
>-		control_writeulong(CONTROL_CONTINUE);
>+		if (send_control)
>+			control_writeulong(CONTROL_CONTINUE);
>
> 	} while (current_nsec() < tout);
>
>-- 2.50.0
>

LGTM!
Thanks for the fix!

Reviewed-by: Luigi Leonardi <leonardi@redhat.com>

Re: [PATCH net-next] vsock/test: fix test for null ptr deref when transport changes

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  8 Jul 2025 13:17:01 +0200 you wrote:
> From: Stefano Garzarella <sgarzare@redhat.com>
> 
> In test_stream_transport_change_client(), the client sends CONTROL_CONTINUE
> on each iteration, even when connect() is unsuccessful. This causes a flood
> of control messages in the server that hangs around for more than 10
> seconds after the test finishes, triggering several timeouts and causing
> subsequent tests to fail. This was discovered in testing a newly proposed
> test that failed in this way on the client side:
>     ...
>     33 - SOCK_STREAM transport change null-ptr-deref...ok
>     34 - SOCK_STREAM ioctl(SIOCINQ) functionality...recv timed out
> 
> [...]

Here is the summary with links:
  - [net-next] vsock/test: fix test for null ptr deref when transport changes
    https://git.kernel.org/netdev/net-next/c/5d6fc6b4d0b2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html