| 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
| ||
|
Message-ID: <grwpjitnh6jqlougrw2b5xibuclos3tpgyxv5exgcbnvcy6crp@jt4o2hz6pk6n>
Date: Wed, 29 Nov 2023 10:36:48 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Arseniy Krasnov <avkrasnov@...utedevices.com>
Cc: Stefan Hajnoczi <stefanha@...hat.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
"Michael S. Tsirkin" <mst@...hat.com>,
Jason Wang <jasowang@...hat.com>,
Bobby Eshleman <bobby.eshleman@...edance.com>,
kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
kernel@...rdevices.ru, oxffffaa@...il.com
Subject: Re: [RFC PATCH v3 3/3] vsock/test: SO_RCVLOWAT + deferred credit
update test
On Wed, Nov 29, 2023 at 12:16:54PM +0300, Arseniy Krasnov wrote:
>
>
>On 29.11.2023 12:16, Stefano Garzarella wrote:
>> On Wed, Nov 22, 2023 at 09:05:10PM +0300, Arseniy Krasnov wrote:
>>> Test which checks, that updating SO_RCVLOWAT value also sends credit
>>> update message. Otherwise mutual hungup may happen when receiver didn't
>>> send credit update and then calls 'poll()' with non default SO_RCVLOWAT
>>> value (e.g. waiting enough bytes to read), while sender waits for free
>>> space at receiver's side. Important thing is that this test relies on
>>> kernel's define for maximum packet size for virtio transport and this
>>> value is not exported to user: VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (this
>>> define is used to control moment when to send credit update message).
>>> If this value or its usage will be changed in kernel - this test may
>>> become useless/broken.
>>>
>>> Signed-off-by: Arseniy Krasnov <avkrasnov@...utedevices.com>
>>> ---
>>> Changelog:
>>> v1 -> v2:
>>> * Update commit message by removing 'This patch adds XXX' manner.
>>> * Update commit message by adding details about dependency for this
>>> test from kernel internal define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE.
>>> * Add comment for this dependency in 'vsock_test.c' where this define
>>> is duplicated.
>>> v2 -> v3:
>>> * Replace synchronization based on control TCP socket with vsock
>>> data socket - this is needed to allow sender transmit data only
>>> when new buffer size of receiver is visible to sender. Otherwise
>>> there is race and test fails sometimes.
>>>
>>> tools/testing/vsock/vsock_test.c | 142 +++++++++++++++++++++++++++++++
>>> 1 file changed, 142 insertions(+)
>>>
>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>> index 5b0e93f9996c..773a71260fba 100644
>>> --- a/tools/testing/vsock/vsock_test.c
>>> +++ b/tools/testing/vsock/vsock_test.c
>>> @@ -1225,6 +1225,143 @@ static void test_double_bind_connect_client(const struct test_opts *opts)
>>> }
>>> }
>>>
>>> +#define RCVLOWAT_CREDIT_UPD_BUF_SIZE (1024 * 128)
>>> +/* This define is the same as in 'include/linux/virtio_vsock.h':
>>> + * it is used to decide when to send credit update message during
>>> + * reading from rx queue of a socket. Value and its usage in
>>> + * kernel is important for this test.
>>> + */
>>> +#define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64)
>>> +
>>> +static void test_stream_rcvlowat_def_cred_upd_client(const struct test_opts *opts)
>>> +{
>>> + size_t buf_size;
>>> + void *buf;
>>> + int fd;
>>> +
>>> + fd = vsock_stream_connect(opts->peer_cid, 1234);
>>> + if (fd < 0) {
>>> + perror("connect");
>>> + exit(EXIT_FAILURE);
>>> + }
>>> +
>>> + /* Send 1 byte more than peer's buffer size. */
>>> + buf_size = RCVLOWAT_CREDIT_UPD_BUF_SIZE + 1;
>>> +
>>> + buf = malloc(buf_size);
>>> + if (!buf) {
>>> + perror("malloc");
>>> + exit(EXIT_FAILURE);
>>> + }
>>> +
>>> + /* Wait until peer sets needed buffer size. */
>>> + recv_byte(fd, 1, 0);
>>> +
>>> + if (send(fd, buf, buf_size, 0) != buf_size) {
>>> + perror("send failed");
>>> + exit(EXIT_FAILURE);
>>> + }
>>> +
>>> + free(buf);
>>> + close(fd);
>>> +}
>>> +
>>> +static void test_stream_rcvlowat_def_cred_upd_server(const struct test_opts *opts)
>>> +{
>>> + size_t recv_buf_size;
>>> + struct pollfd fds;
>>> + size_t buf_size;
>>> + void *buf;
>>> + int fd;
>>> +
>>> + fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL);
>>> + if (fd < 0) {
>>> + perror("accept");
>>> + exit(EXIT_FAILURE);
>>> + }
>>> +
>>> + buf_size = RCVLOWAT_CREDIT_UPD_BUF_SIZE;
>>> +
>>> + if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>>> + &buf_size, sizeof(buf_size))) {
>>> + perror("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
>>> + exit(EXIT_FAILURE);
>>> + }
>>> +
>>> + /* Send one dummy byte here, because 'setsockopt()' above also
>>> + * sends special packet which tells sender to update our buffer
>>> + * size. This 'send_byte()' will serialize such packet with data
>>> + * reads in a loop below. Sender starts transmission only when
>>> + * it receives this single byte.
>>> + */
>>> + send_byte(fd, 1, 0);
>>> +
>>> + buf = malloc(buf_size);
>>> + if (!buf) {
>>> + perror("malloc");
>>> + exit(EXIT_FAILURE);
>>> + }
>>> +
>>> + /* Wait until there will be 128KB of data in rx queue. */
>>> + while (1) {
>>> + ssize_t res;
>>> +
>>> + res = recv(fd, buf, buf_size, MSG_PEEK);
>>> + if (res == buf_size)
>>> + break;
>>> +
>>> + if (res <= 0) {
>>> + fprintf(stderr, "unexpected 'recv()' return: %zi\n", res);
>>> + exit(EXIT_FAILURE);
>>> + }
>>> + }
>>> +
>>> + /* There is 128KB of data in the socket's rx queue,
>>> + * dequeue first 64KB, credit update is not sent.
>>> + */
>>> + recv_buf_size = VIRTIO_VSOCK_MAX_PKT_BUF_SIZE;
>>> + recv_buf(fd, buf, recv_buf_size, 0, recv_buf_size);
>>> + recv_buf_size++;
>>> +
>>> + /* Updating SO_RCVLOWAT will send credit update. */
>>> + if (setsockopt(fd, SOL_SOCKET, SO_RCVLOWAT,
>>> + &recv_buf_size, sizeof(recv_buf_size))) {
>>> + perror("setsockopt(SO_RCVLOWAT)");
>>> + exit(EXIT_FAILURE);
>>> + }
>>> +
>>> + memset(&fds, 0, sizeof(fds));
>>> + fds.fd = fd;
>>> + fds.events = POLLIN | POLLRDNORM | POLLERR |
>>> + POLLRDHUP | POLLHUP;
>>> +
>>> + /* This 'poll()' will return once we receive last byte
>>> + * sent by client.
>>> + */
>>> + if (poll(&fds, 1, -1) < 0) {
>>> + perror("poll");
>>> + exit(EXIT_FAILURE);
>>> + }
>>> +
>>> + if (fds.revents & POLLERR) {
>>> + fprintf(stderr, "'poll()' error\n");
>>> + exit(EXIT_FAILURE);
>>> + }
>>> +
>>> + if (fds.revents & (POLLIN | POLLRDNORM)) {
>>> + recv_buf(fd, buf, recv_buf_size, 0, recv_buf_size);
>>
>> Should we set the socket non-blocking?
>>
>> Otherwise, here poll() might wake up even if there are not all the
>> expected bytes due to some bug and recv() block waiting for the
>> remaining bytes, so we might not notice the bug.
>
>Good point! or just use MSG_DONTWAIT flag for only this 'recv()'.
Yep, right!
Stefano
Powered by blists - more mailing lists