| Message ID | 20241023210031.274017-1-kshk@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | vsock/test: fix failures due to wrong SO_RCVLOWAT parameter | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Oct 23, 2024 at 04:00:31PM -0500, Konstantin Shkolnyy wrote: >This happens on 64-bit big-endian machines. >SO_RCVLOWAT requires an int parameter. However, instead of int, the test >uses unsigned long in one place and size_t in another. Both are 8 bytes >long on 64-bit machines. The kernel, having received the 8 bytes, doesn't >test for the exact size of the parameter, it only cares that it's >= >sizeof(int), and casts the 4 lower-addressed bytes to an int, which, on >a big-endian machine, contains 0. 0 doesn't trigger an error, SO_RCVLOWAT >returns with success and the socket stays with the default SO_RCVLOWAT = 1, >which results in test failures. > >Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com> >--- > >Notes: > The problem was found on s390 (big endian), while x86-64 didn't show it. After this fix, all tests pass on s390. Thanks for the fix! Other setsockopt() in the tests where we use unsigned long are SO_VM_SOCKETS_* but they are expected to be unsigned, so we should be fine. Not for this patch, but do you think adding a getsockopt() for each setsockopt in the test to check that kind of issue can help? BTW, this patch LGTM: Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Not sure if we want this with net tree since are just tests, in that case I think you should add: Fixes: b1346338fbae ("vsock_test: POLLIN + SO_RCVLOWAT test") Fixes: 542e893fbadc ("vsock/test: two tests to check credit update logic") > > tools/testing/vsock/vsock_test.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >index 8d38dbf8f41f..7fd25b814b4b 100644 >--- a/tools/testing/vsock/vsock_test.c >+++ b/tools/testing/vsock/vsock_test.c >@@ -835,7 +835,7 @@ static void test_stream_poll_rcvlowat_server(const struct test_opts *opts) > > static void test_stream_poll_rcvlowat_client(const struct test_opts *opts) > { >- unsigned long lowat_val = RCVLOWAT_BUF_SIZE; >+ int lowat_val = RCVLOWAT_BUF_SIZE; > char buf[RCVLOWAT_BUF_SIZE]; > struct pollfd fds; > short poll_flags; >@@ -1357,7 +1357,7 @@ static void test_stream_rcvlowat_def_cred_upd_client(const struct test_opts *opt > static void test_stream_credit_update_test(const struct test_opts *opts, > bool low_rx_bytes_test) > { >- size_t recv_buf_size; >+ int recv_buf_size; > struct pollfd fds; > size_t buf_size; > void *buf; >-- >2.34.1 >
On 10/24/2024 03:43, Stefano Garzarella wrote: > Other setsockopt() in the tests where we use unsigned long are > SO_VM_SOCKETS_* but they are expected to be unsigned, so we should be > fine. It's actually not "signed vs unsigned", but a "size + endianess" problem. Also, looking at SO_VM_SOCKETS_* code in the test, it uses unsigned long and size_t which (I believe) will both shrink to 4 bytes on 32-bit machines, while the corresponding kernel code in af_vsock.c uses u64. It looks to me that this kernel code will be unhappy to receive just 4 bytes when it expects 8.
On Thu, Oct 24, 2024 at 10:00:47AM -0500, Konstantin Shkolnyy wrote: >On 10/24/2024 03:43, Stefano Garzarella wrote: >>Other setsockopt() in the tests where we use unsigned long are >>SO_VM_SOCKETS_* but they are expected to be unsigned, so we should be >>fine. > >It's actually not "signed vs unsigned", but a "size + endianess" problem. I see, thanks! > >Also, looking at SO_VM_SOCKETS_* code in the test, it uses unsigned >long and size_t which (I believe) will both shrink to 4 bytes on 32-bit >machines, while the corresponding kernel code in af_vsock.c uses u64. >It looks to me that this kernel code will be unhappy to receive just 4 >bytes when it expects 8. > In include/uapi/linux/vm_sockets.h we talk about unsigned long long for SO_VM_SOCKETS_*, that IIUC also on 32-bit machines should be on 64bit, so the kernel code looks okay, but the tests should be improved, right? Thanks, Stefano
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c index 8d38dbf8f41f..7fd25b814b4b 100644 --- a/tools/testing/vsock/vsock_test.c +++ b/tools/testing/vsock/vsock_test.c @@ -835,7 +835,7 @@ static void test_stream_poll_rcvlowat_server(const struct test_opts *opts) static void test_stream_poll_rcvlowat_client(const struct test_opts *opts) { - unsigned long lowat_val = RCVLOWAT_BUF_SIZE; + int lowat_val = RCVLOWAT_BUF_SIZE; char buf[RCVLOWAT_BUF_SIZE]; struct pollfd fds; short poll_flags; @@ -1357,7 +1357,7 @@ static void test_stream_rcvlowat_def_cred_upd_client(const struct test_opts *opt static void test_stream_credit_update_test(const struct test_opts *opts, bool low_rx_bytes_test) { - size_t recv_buf_size; + int recv_buf_size; struct pollfd fds; size_t buf_size; void *buf;
This happens on 64-bit big-endian machines. SO_RCVLOWAT requires an int parameter. However, instead of int, the test uses unsigned long in one place and size_t in another. Both are 8 bytes long on 64-bit machines. The kernel, having received the 8 bytes, doesn't test for the exact size of the parameter, it only cares that it's >= sizeof(int), and casts the 4 lower-addressed bytes to an int, which, on a big-endian machine, contains 0. 0 doesn't trigger an error, SO_RCVLOWAT returns with success and the socket stays with the default SO_RCVLOWAT = 1, which results in test failures. Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com> --- Notes: The problem was found on s390 (big endian), while x86-64 didn't show it. After this fix, all tests pass on s390. tools/testing/vsock/vsock_test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)