| 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: <be8b1d3e-7032-76dc-042c-9513a933a0f3@salutedevices.com>
Date: Wed, 29 Nov 2023 12:16:54 +0300
From: Arseniy Krasnov <avkrasnov@...utedevices.com>
To: Stefano Garzarella <sgarzare@...hat.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 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()'.
Thanks, Arseniy
>
> Stefano
>
>> + } else {
>> + /* These flags must be set, as there is at
>> + * least 64KB of data ready to read.
>> + */
>> + fprintf(stderr, "POLLIN | POLLRDNORM expected\n");
>> + exit(EXIT_FAILURE);
>> + }
>> +
>> + free(buf);
>> + close(fd);
>> +}
>> +
>> static struct test_case test_cases[] = {
>> {
>> .name = "SOCK_STREAM connection reset",
>> @@ -1335,6 +1472,11 @@ static struct test_case test_cases[] = {
>> .run_client = test_double_bind_connect_client,
>> .run_server = test_double_bind_connect_server,
>> },
>> + {
>> + .name = "SOCK_STREAM virtio SO_RCVLOWAT + deferred cred update",
>> + .run_client = test_stream_rcvlowat_def_cred_upd_client,
>> + .run_server = test_stream_rcvlowat_def_cred_upd_server,
>> + },
>> {},
>> };
>>
>> --
>> 2.25.1
>>
>
Powered by blists - more mailing lists