| Message ID | 20241029144954.285279-3-kshk@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | vsock/test: fix wrong setsockopt() parameters | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 >
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"?
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
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); }
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(-)