[v4] vsock/test: fix wrong setsockopt() parameters

[PATCH v4 2/2] vsock/test: fix parameter types in SO_VM_SOCKETS_* calls

Posted by Konstantin Shkolnyy 3 weeks, 5 days ago

Change parameters of SO_VM_SOCKETS_* to uint64_t so that they are always
64-bit, because the corresponding kernel code requires them to be at least
that large, no matter what architecture.

Fixes: 5c338112e48a ("test/vsock: rework message bounds test")
Fixes: 685a21c314a8 ("test/vsock: add big message test")
Fixes: 542e893fbadc ("vsock/test: two tests to check credit update logic")
Fixes: 8abbffd27ced ("test/vsock: vsock_perf utility")
Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
---
 tools/testing/vsock/vsock_perf.c |  2 +-
 tools/testing/vsock/vsock_test.c | 19 ++++++++++++++-----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c
index 22633c2848cc..88f6be4162a6 100644
--- a/tools/testing/vsock/vsock_perf.c
+++ b/tools/testing/vsock/vsock_perf.c
@@ -33,7 +33,7 @@
 
 static unsigned int port = DEFAULT_PORT;
 static unsigned long buf_size_bytes = DEFAULT_BUF_SIZE_BYTES;
-static unsigned long vsock_buf_bytes = DEFAULT_VSOCK_BUF_BYTES;
+static uint64_t vsock_buf_bytes = DEFAULT_VSOCK_BUF_BYTES;
 static bool zerocopy;
 
 static void error(const char *s)
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 7fd25b814b4b..49a32515886f 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -429,7 +429,7 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
 
 static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
 {
-	unsigned long sock_buf_size;
+	uint64_t sock_buf_size;
 	unsigned long remote_hash;
 	unsigned long curr_hash;
 	int fd;
@@ -634,7 +634,8 @@ static void test_seqpacket_timeout_server(const struct test_opts *opts)
 
 static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
 {
-	unsigned long sock_buf_size;
+	uint64_t sock_buf_size;
+	size_t buf_size;
 	socklen_t len;
 	void *data;
 	int fd;
@@ -655,13 +656,19 @@ static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
 
 	sock_buf_size++;
 
-	data = malloc(sock_buf_size);
+	buf_size = (size_t) sock_buf_size; /* size_t can be < uint64_t */
+	if (buf_size != sock_buf_size) {
+		fprintf(stderr, "Returned BUFFER_SIZE too large\n");
+		exit(EXIT_FAILURE);
+	}
+
+	data = malloc(buf_size);
 	if (!data) {
 		perror("malloc");
 		exit(EXIT_FAILURE);
 	}
 
-	send_buf(fd, data, sock_buf_size, 0, -EMSGSIZE);
+	send_buf(fd, data, buf_size, 0, -EMSGSIZE);
 
 	control_writeln("CLISENT");
 
@@ -1360,6 +1367,7 @@ static void test_stream_credit_update_test(const struct test_opts *opts,
 	int recv_buf_size;
 	struct pollfd fds;
 	size_t buf_size;
+	uint64_t sock_buf_size;
 	void *buf;
 	int fd;
 
@@ -1370,9 +1378,10 @@ static void test_stream_credit_update_test(const struct test_opts *opts,
 	}
 
 	buf_size = RCVLOWAT_CREDIT_UPD_BUF_SIZE;
+	sock_buf_size = buf_size; /* size_t can be < uint64_t */
 
 	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
-		       &buf_size, sizeof(buf_size))) {
+		       &sock_buf_size, sizeof(sock_buf_size))) {
 		perror("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
 		exit(EXIT_FAILURE);
 	}
-- 
2.34.1

Re: [PATCH v4 2/2] vsock/test: fix parameter types in SO_VM_SOCKETS_* calls

Posted by Stefano Garzarella 3 weeks, 3 days ago

On Tue, Oct 29, 2024 at 09:49:54AM -0500, Konstantin Shkolnyy wrote:
>Change parameters of SO_VM_SOCKETS_* to uint64_t so that they are always

In include/uapi/linux/vm_sockets.h we talk about "unsigned long long",
but in the kernel code we use u64. IIUC "unsigned long long" should be 
u64 on every architecture, at least till we will have some 128-bit cpu, 
right?

>64-bit, because the corresponding kernel code requires them to be at 
>least
>that large, no matter what architecture.
>
>Fixes: 5c338112e48a ("test/vsock: rework message bounds test")
>Fixes: 685a21c314a8 ("test/vsock: add big message test")
>Fixes: 542e893fbadc ("vsock/test: two tests to check credit update logic")
>Fixes: 8abbffd27ced ("test/vsock: vsock_perf utility")
>Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
>---
> tools/testing/vsock/vsock_perf.c |  2 +-
> tools/testing/vsock/vsock_test.c | 19 ++++++++++++++-----
> 2 files changed, 15 insertions(+), 6 deletions(-)
>
>diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c
>index 22633c2848cc..88f6be4162a6 100644
>--- a/tools/testing/vsock/vsock_perf.c
>+++ b/tools/testing/vsock/vsock_perf.c
>@@ -33,7 +33,7 @@
>
> static unsigned int port = DEFAULT_PORT;
> static unsigned long buf_size_bytes = DEFAULT_BUF_SIZE_BYTES;
>-static unsigned long vsock_buf_bytes = DEFAULT_VSOCK_BUF_BYTES;
>+static uint64_t vsock_buf_bytes = DEFAULT_VSOCK_BUF_BYTES;

What about using `unsigned long long` as documented in the vm_sockets.h?

Thanks,
Stefano

> static bool zerocopy;
>
> static void error(const char *s)
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 7fd25b814b4b..49a32515886f 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -429,7 +429,7 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>
> static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
> {
>-	unsigned long sock_buf_size;
>+	uint64_t sock_buf_size;
> 	unsigned long remote_hash;
> 	unsigned long curr_hash;
> 	int fd;
>@@ -634,7 +634,8 @@ static void test_seqpacket_timeout_server(const struct test_opts *opts)
>
> static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
> {
>-	unsigned long sock_buf_size;
>+	uint64_t sock_buf_size;
>+	size_t buf_size;
> 	socklen_t len;
> 	void *data;
> 	int fd;
>@@ -655,13 +656,19 @@ static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
>
> 	sock_buf_size++;
>
>-	data = malloc(sock_buf_size);
>+	buf_size = (size_t) sock_buf_size; /* size_t can be < uint64_t */
>+	if (buf_size != sock_buf_size) {
>+		fprintf(stderr, "Returned BUFFER_SIZE too large\n");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	data = malloc(buf_size);
> 	if (!data) {
> 		perror("malloc");
> 		exit(EXIT_FAILURE);
> 	}
>
>-	send_buf(fd, data, sock_buf_size, 0, -EMSGSIZE);
>+	send_buf(fd, data, buf_size, 0, -EMSGSIZE);
>
> 	control_writeln("CLISENT");
>
>@@ -1360,6 +1367,7 @@ static void test_stream_credit_update_test(const struct test_opts *opts,
> 	int recv_buf_size;
> 	struct pollfd fds;
> 	size_t buf_size;
>+	uint64_t sock_buf_size;
> 	void *buf;
> 	int fd;
>
>@@ -1370,9 +1378,10 @@ static void test_stream_credit_update_test(const struct test_opts *opts,
> 	}
>
> 	buf_size = RCVLOWAT_CREDIT_UPD_BUF_SIZE;
>+	sock_buf_size = buf_size; /* size_t can be < uint64_t */
>
> 	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>-		       &buf_size, sizeof(buf_size))) {
>+		       &sock_buf_size, sizeof(sock_buf_size))) {
> 		perror("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
> 		exit(EXIT_FAILURE);
> 	}
>-- 
>2.34.1
>

Re: [PATCH v4 2/2] vsock/test: fix parameter types in SO_VM_SOCKETS_* calls

Posted by Konstantin Shkolnyy 3 weeks, 3 days ago

On 10/31/2024 09:16, Stefano Garzarella wrote:
> On Tue, Oct 29, 2024 at 09:49:54AM -0500, Konstantin Shkolnyy wrote:
>> Change parameters of SO_VM_SOCKETS_* to uint64_t so that they are always
> 
> In include/uapi/linux/vm_sockets.h we talk about "unsigned long long",
> but in the kernel code we use u64. IIUC "unsigned long long" should be 
> u64 on every architecture, at least till we will have some 128-bit cpu, 
> right?

I'm not sure what "unsigned long long" would be on a 128-bit machine.

> What about using `unsigned long long` as documented in the vm_sockets.h?

I use uint64_t because the kernel uses u64. I think, this way the code
isn't vulnerable to potential variability of "unsigned long long".
If we change to "unsigned long long" should we also change the kernel
to "unsigned long long"?

Re: [PATCH v4 2/2] vsock/test: fix parameter types in SO_VM_SOCKETS_* calls

Posted by Stefano Garzarella 2 weeks, 6 days ago

On Thu, Oct 31, 2024 at 11:04:06AM -0500, Konstantin Shkolnyy wrote:
>On 10/31/2024 09:16, Stefano Garzarella wrote:
>>On Tue, Oct 29, 2024 at 09:49:54AM -0500, Konstantin Shkolnyy wrote:
>>>Change parameters of SO_VM_SOCKETS_* to uint64_t so that they are always
>>
>>In include/uapi/linux/vm_sockets.h we talk about "unsigned long long",
>>but in the kernel code we use u64. IIUC "unsigned long long" should 
>>be u64 on every architecture, at least till we will have some 
>>128-bit cpu, right?
>
>I'm not sure what "unsigned long long" would be on a 128-bit machine.
>
>>What about using `unsigned long long` as documented in the vm_sockets.h?
>
>I use uint64_t because the kernel uses u64. I think, this way the code
>isn't vulnerable to potential variability of "unsigned long long".

IMHO the test should look more at UAPI than implementation.
Since we document to use "unsigned long long" I think we should use that 
in the test to check that our implementation behaves well with what we 
suggest the user to do.

>If we change to "unsigned long long" should we also change the kernel
>to "unsigned long long"?
>

For now, it should not change much to use u64 or unsigned long long, but 
I agree that it would be better to change it. I would do it in a 
separate series, though.

Thanks,
Stefano