linux-kernel - Re: [PATCH net] vsock/test: Fix occasional failure in SIOCOUTQ tests

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] [day] [month] [year] [list]

Message-ID: <sqee4iqviojcht4s42dke3mnsq4f4si6oislu77bm3nqwlowim@oz6voimaqw4m>
Date: Wed, 7 May 2025 15:08:22 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Konstantin Shkolnyy <kshk@...ux.ibm.com>
Cc: virtualization@...ts.linux.dev, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, mjrosato@...ux.ibm.com
Subject: Re: [PATCH net] vsock/test: Fix occasional failure in SIOCOUTQ tests

On Wed, May 07, 2025 at 06:48:33AM -0500, Konstantin Shkolnyy wrote:
>These tests:
>    "SOCK_STREAM ioctl(SIOCOUTQ) 0 unsent bytes"
>    "SOCK_SEQPACKET ioctl(SIOCOUTQ) 0 unsent bytes"
>output: "Unexpected 'SIOCOUTQ' value, expected 0, got 64 (CLIENT)".
>
>They test that the SIOCOUTQ ioctl reports 0 unsent bytes after the data
>have been received by the other side. However, sometimes there is a delay
>in updating this "unsent bytes" counter, and the test fails even though
>the counter properly goes to 0 several milliseconds later.
>
>The delay occurs in the kernel because the used buffer notification
>callback virtio_vsock_tx_done(), called upon receipt of the data by the
>other side, doesn't update the counter itself. It delegates that to
>a kernel thread (via vsock->tx_work). Sometimes that thread is delayed
>more than the test expects.
>
>Change the test to try SIOCOUTQ several times with small delays in between.
>
>Signed-off-by: Konstantin Shkolnyy <kshk@...ux.ibm.com>
>---
> tools/testing/vsock/vsock_test.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index d0f6d253ac72..143f1cba2d18 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -1264,21 +1264,27 @@ static void test_unsent_bytes_client(const struct test_opts *opts, int type)
> 	send_buf(fd, buf, sizeof(buf), 0, sizeof(buf));
> 	control_expectln("RECEIVED");
>
>-	ret = ioctl(fd, SIOCOUTQ, &sock_bytes_unsent);
>-	if (ret < 0) {
>-		if (errno == EOPNOTSUPP) {
>-			fprintf(stderr, "Test skipped, SIOCOUTQ not supported.\n");
>-		} else {
>+	/* SIOCOUTQ isn't guaranteed to instantly track sent data */
>+	for (int i = 0; i < 10; i++) {
>+		ret = ioctl(fd, SIOCOUTQ, &sock_bytes_unsent);
>+		if (ret == 0 && sock_bytes_unsent == 0)
>+			goto success;
>+
>+		if (ret < 0) {
>+			if (errno == EOPNOTSUPP) {
>+				fprintf(stderr, "Test skipped, SIOCOUTQ not supported.\n");
>+				goto success;
>+			}
> 			perror("ioctl");
> 			exit(EXIT_FAILURE);
> 		}
>-	} else if (ret == 0 && sock_bytes_unsent != 0) {
>-		fprintf(stderr,
>-			"Unexpected 'SIOCOUTQ' value, expected 0, got %i\n",
>-			sock_bytes_unsent);
>-		exit(EXIT_FAILURE);
>+		usleep(10 * 1000);
> 	}
>
>+	fprintf(stderr, "Unexpected 'SIOCOUTQ' value, expected 0, got %i\n",
>+		sock_bytes_unsent);
>+	exit(EXIT_FAILURE);
>+success:
> 	close(fd);

I worked on something similar but I didn't yet send it.

I like the delay you put, but I prefer to use the timeout stuff we have
to retry, like I did here:

@@ -1264,20 +1270,25 @@ static void test_unsent_bytes_client(const struct test_opts *op
ts, int type)
         send_buf(fd, buf, sizeof(buf), 0, sizeof(buf));
         control_expectln("RECEIVED");

-       ret = ioctl(fd, SIOCOUTQ, &sock_bytes_unsent);
-       if (ret < 0) {
-               if (errno == EOPNOTSUPP) {
-                       fprintf(stderr, "Test skipped, SIOCOUTQ not supported.\n");
-               } else {
-                       perror("ioctl");
-                       exit(EXIT_FAILURE);
+       /* Although we have a control message, we are not sure that the vsock
+        * transport has sent us notification that the buffer has been copied
+        * and cleared, so in some cases we may still see unsent bytes.
+        * Better to do a few iterations to be sure.
+        */
+       timeout_begin(TIMEOUT);
+       do {
+               ret = ioctl(fd, SIOCOUTQ, &sock_bytes_unsent);
+               if (ret < 0) {
+                       if (errno == EOPNOTSUPP) {
+                               fprintf(stderr, "Test skipped, SIOCOUTQ not supported.\n");
+                               break;
+                       } else {
+                               perror("ioctl");
+                               exit(EXIT_FAILURE);
+                       }
                 }
-       } else if (ret == 0 && sock_bytes_unsent != 0) {
-               fprintf(stderr,
-                       "Unexpected 'SIOCOUTQ' value, expected 0, got %i\n",
-                       sock_bytes_unsent);
-               exit(EXIT_FAILURE);
-       }
+       } while (sock_bytes_unsent != 0);
+       timeout_end();


What about combining the two?

Thanks,
Stefano