This patchset modifies receive logic for SOCK_SEQPACKET.
Difference between current implementation and this version is that
now reader is woken up when there is at least one RW packet in rx
queue of socket and data is copied to user's buffer, while merged
approach wake up user only when whole message is received and kept
in queue. New implementation has several advantages:
1) There is no limit for message length. Merged approach requires
that length must be smaller than 'peer_buf_alloc', otherwise
transmission will stuck.
2) There is no need to keep whole message in queue, thus no
'kmalloc()' memory will be wasted until EOR is received.
Also new approach has some feature: as fragments of message
are copied until EOR is received, it is possible that part of
message will be already in user's buffer, while rest of message
still not received. And if user will be interrupted by signal or
timeout with part of message in buffer, it will exit receive loop,
leaving rest of message in queue. To solve this problem special
callback was added to transport: it is called when user was forced
to leave exit loop and tells transport to drop any packet until
EOR met. When EOR is found, this mode is disabled and normal packet
processing started. Note, that when 'drop until EOR' mode is on,
incoming packets still inserted in queue, reader will be woken up,
tries to copy data, but nothing will be copied until EOR found.
It was possible to drain such unneeded packets it rx work without
kicking user, but implemented way is simplest. Anyway, i think
such cases are rare.
New test also added - it tries to copy to invalid user's
buffer.
Arseny Krasnov (16):
vhost/vsock: don't set 'seqpacket_has_data()' callback
vsock/loopback: don't set 'seqpacket_has_data()' callback
virtio/vsock: don't set 'seqpacket_has_data()' callback
virtio/vsock: remove 'virtio_transport_seqpacket_has_data'
af_vsock: use SOCK_STREAM function to check data
vsock/virtio: remove record size limit for SEQPACKET
virtio/vsock: don't count EORs on receive
af_vsock: change SEQPACKET receive loop
af_vsock/virtio: update dequeue callback interface
virtio/vsock: update SEQPACKET dequeue logic
afvsock: add 'seqpacket_drop()'
virtio/vsock: add 'drop until EOR' logic
vhost/vsock: enable 'seqpacket_drop' callback in transport
virtio/vsock: enable 'seqpacket_drop' callback in transport
vsock/loopback: enable 'seqpacket_drop' callback in transport
vsock_test: SEQPACKET read to broken buffer
drivers/vhost/vsock.c | 2 +-
include/linux/virtio_vsock.h | 7 +-
include/net/af_vsock.h | 4 +-
net/vmw_vsock/af_vsock.c | 44 ++++----
net/vmw_vsock/virtio_transport.c | 2 +-
net/vmw_vsock/virtio_transport_common.c | 103 +++++++-----------
net/vmw_vsock/vsock_loopback.c | 2 +-
tools/testing/vsock/vsock_test.c | 121 ++++++++++++++++++++++
8 files changed, 194 insertions(+), 91 deletions(-)
Signed-off-by: Arseny Krasnov <[email protected]>
--
2.25.1
As now 'rx_bytes' is used to check presence of data on socket,
this function is obsolete.
Signed-off-by: Arseny Krasnov <[email protected]>
---
include/linux/virtio_vsock.h | 1 -
net/vmw_vsock/virtio_transport_common.c | 13 -------------
2 files changed, 14 deletions(-)
diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index 35d7eedb5e8e..719008d4235e 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h
@@ -91,7 +91,6 @@ virtio_transport_seqpacket_dequeue(struct vsock_sock *vsk,
int flags);
s64 virtio_transport_stream_has_data(struct vsock_sock *vsk);
s64 virtio_transport_stream_has_space(struct vsock_sock *vsk);
-u32 virtio_transport_seqpacket_has_data(struct vsock_sock *vsk);
int virtio_transport_do_socket_init(struct vsock_sock *vsk,
struct vsock_sock *psk);
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index f014ccfdd9c2..bc25961509e0 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -540,19 +540,6 @@ s64 virtio_transport_stream_has_data(struct vsock_sock *vsk)
}
EXPORT_SYMBOL_GPL(virtio_transport_stream_has_data);
-u32 virtio_transport_seqpacket_has_data(struct vsock_sock *vsk)
-{
- struct virtio_vsock_sock *vvs = vsk->trans;
- u32 msg_count;
-
- spin_lock_bh(&vvs->rx_lock);
- msg_count = vvs->msg_count;
- spin_unlock_bh(&vvs->rx_lock);
-
- return msg_count;
-}
-EXPORT_SYMBOL_GPL(virtio_transport_seqpacket_has_data);
-
static s64 virtio_transport_has_space(struct vsock_sock *vsk)
{
struct virtio_vsock_sock *vvs = vsk->trans;
--
2.25.1
Also remove 'seqpacket_has_data' callback from transport.
Signed-off-by: Arseny Krasnov <[email protected]>
---
include/net/af_vsock.h | 1 -
net/vmw_vsock/af_vsock.c | 12 +-----------
2 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index ab207677e0a8..bf5ea1873e6f 100644
--- a/include/net/af_vsock.h
+++ b/include/net/af_vsock.h
@@ -141,7 +141,6 @@ struct vsock_transport {
int (*seqpacket_enqueue)(struct vsock_sock *vsk, struct msghdr *msg,
size_t len);
bool (*seqpacket_allow)(u32 remote_cid);
- u32 (*seqpacket_has_data)(struct vsock_sock *vsk);
/* Notification. */
int (*notify_poll_in)(struct vsock_sock *, size_t, bool *);
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 21ccf450e249..59ce35da2e5b 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -860,16 +860,6 @@ s64 vsock_stream_has_data(struct vsock_sock *vsk)
}
EXPORT_SYMBOL_GPL(vsock_stream_has_data);
-static s64 vsock_connectible_has_data(struct vsock_sock *vsk)
-{
- struct sock *sk = sk_vsock(vsk);
-
- if (sk->sk_type == SOCK_SEQPACKET)
- return vsk->transport->seqpacket_has_data(vsk);
- else
- return vsock_stream_has_data(vsk);
-}
-
s64 vsock_stream_has_space(struct vsock_sock *vsk)
{
return vsk->transport->stream_has_space(vsk);
@@ -1881,7 +1871,7 @@ static int vsock_connectible_wait_data(struct sock *sk,
err = 0;
transport = vsk->transport;
- while ((data = vsock_connectible_has_data(vsk)) == 0) {
+ while ((data = vsock_stream_has_data(vsk)) == 0) {
prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE);
if (sk->sk_err != 0 ||
--
2.25.1
Clean 'seqpacket_has_data()' callback in transport struct.
Signed-off-by: Arseny Krasnov <[email protected]>
---
net/vmw_vsock/virtio_transport.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index ed1664e7bd88..e8b8108f3a29 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -475,7 +475,6 @@ static struct virtio_transport virtio_transport = {
.seqpacket_dequeue = virtio_transport_seqpacket_dequeue,
.seqpacket_enqueue = virtio_transport_seqpacket_enqueue,
.seqpacket_allow = virtio_transport_seqpacket_allow,
- .seqpacket_has_data = virtio_transport_seqpacket_has_data,
.notify_poll_in = virtio_transport_notify_poll_in,
.notify_poll_out = virtio_transport_notify_poll_out,
--
2.25.1
Remove record size limit which was 'peer_buf_alloc' value.
New approach doesn't need this, because data is copied to
user's buffer in stream manner(we don't wait until whole
record is received).
Signed-off-by: Arseny Krasnov <[email protected]>
---
net/vmw_vsock/virtio_transport_common.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index bc25961509e0..84431d7a87a5 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -503,17 +503,6 @@ virtio_transport_seqpacket_enqueue(struct vsock_sock *vsk,
struct msghdr *msg,
size_t len)
{
- struct virtio_vsock_sock *vvs = vsk->trans;
-
- spin_lock_bh(&vvs->tx_lock);
-
- if (len > vvs->peer_buf_alloc) {
- spin_unlock_bh(&vvs->tx_lock);
- return -EMSGSIZE;
- }
-
- spin_unlock_bh(&vvs->tx_lock);
-
return virtio_transport_stream_enqueue(vsk, msg, len);
}
EXPORT_SYMBOL_GPL(virtio_transport_seqpacket_enqueue);
--
2.25.1
As message copied by fragments, in addition to EOR met,
dequeue loop iterates until queue will be empty or copy
error found.
Signed-off-by: Arseny Krasnov <[email protected]>
---
include/linux/virtio_vsock.h | 1 -
net/vmw_vsock/virtio_transport_common.c | 61 ++++++++++---------------
2 files changed, 25 insertions(+), 37 deletions(-)
diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index 8d34f3d73bbb..7360ab7ea0af 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h
@@ -36,7 +36,6 @@ struct virtio_vsock_sock {
u32 rx_bytes;
u32 buf_alloc;
struct list_head rx_queue;
- u32 msg_count;
};
struct virtio_vsock_pkt {
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 9c2bd84ab8e6..5a46c3f94e83 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -407,59 +407,48 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
struct msghdr *msg,
- int flags)
+ int flags,
+ bool *msg_ready)
{
struct virtio_vsock_sock *vvs = vsk->trans;
struct virtio_vsock_pkt *pkt;
int dequeued_len = 0;
size_t user_buf_len = msg_data_left(msg);
- bool msg_ready = false;
+ *msg_ready = false;
spin_lock_bh(&vvs->rx_lock);
- if (vvs->msg_count == 0) {
- spin_unlock_bh(&vvs->rx_lock);
- return 0;
- }
+ while (!*msg_ready && !list_empty(&vvs->rx_queue) && dequeued_len >= 0) {
+ size_t pkt_len;
+ size_t bytes_to_copy;
- while (!msg_ready) {
pkt = list_first_entry(&vvs->rx_queue, struct virtio_vsock_pkt, list);
+ pkt_len = (size_t)le32_to_cpu(pkt->hdr.len);
- if (dequeued_len >= 0) {
- size_t pkt_len;
- size_t bytes_to_copy;
+ bytes_to_copy = min(user_buf_len, pkt_len);
- pkt_len = (size_t)le32_to_cpu(pkt->hdr.len);
- bytes_to_copy = min(user_buf_len, pkt_len);
-
- if (bytes_to_copy) {
- int err;
-
- /* sk_lock is held by caller so no one else can dequeue.
- * Unlock rx_lock since memcpy_to_msg() may sleep.
- */
- spin_unlock_bh(&vvs->rx_lock);
+ if (bytes_to_copy) {
+ int err;
+ /* sk_lock is held by caller so no one else can dequeue.
+ * Unlock rx_lock since memcpy_to_msg() may sleep.
+ */
+ spin_unlock_bh(&vvs->rx_lock);
- err = memcpy_to_msg(msg, pkt->buf, bytes_to_copy);
- if (err) {
- /* Copy of message failed. Rest of
- * fragments will be freed without copy.
- */
- dequeued_len = err;
- } else {
- user_buf_len -= bytes_to_copy;
- }
+ err = memcpy_to_msg(msg, pkt->buf, bytes_to_copy);
- spin_lock_bh(&vvs->rx_lock);
- }
+ spin_lock_bh(&vvs->rx_lock);
- if (dequeued_len >= 0)
- dequeued_len += pkt_len;
+ if (err)
+ dequeued_len = err;
+ else
+ user_buf_len -= bytes_to_copy;
}
+ if (dequeued_len >= 0)
+ dequeued_len += pkt_len;
+
if (le32_to_cpu(pkt->hdr.flags) & VIRTIO_VSOCK_SEQ_EOR) {
- msg_ready = true;
- vvs->msg_count--;
+ *msg_ready = true;
}
virtio_transport_dec_rx_pkt(vvs, pkt);
@@ -494,7 +483,7 @@ virtio_transport_seqpacket_dequeue(struct vsock_sock *vsk,
if (flags & MSG_PEEK)
return -EOPNOTSUPP;
- return virtio_transport_seqpacket_do_dequeue(vsk, msg, flags);
+ return virtio_transport_seqpacket_do_dequeue(vsk, msg, flags, msg_ready);
}
EXPORT_SYMBOL_GPL(virtio_transport_seqpacket_dequeue);
--
2.25.1
There is no sense to count EORs, because 'rx_bytes' is
used to check data presence on socket.
Signed-off-by: Arseny Krasnov <[email protected]>
---
net/vmw_vsock/virtio_transport_common.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 84431d7a87a5..319c3345f3e0 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1005,9 +1005,6 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
goto out;
}
- if (le32_to_cpu(pkt->hdr.flags) & VIRTIO_VSOCK_SEQ_EOR)
- vvs->msg_count++;
-
/* Try to copy small packets into the buffer of last packet queued,
* to avoid wasting memory queueing the entire buffer with a small
* payload.
--
2.25.1
Add special callback for SEQPACKET socket which is called when
we need to drop current in-progress record: part of record was
copied successfully, reader wait rest of record, but signal
interrupts it and reader leaves it's loop, leaving packets of
current record still in queue. So to avoid copy of "orphaned"
record, we tell transport to drop every packet until EOR will
be found.
Signed-off-by: Arseny Krasnov <[email protected]>
---
include/net/af_vsock.h | 1 +
net/vmw_vsock/af_vsock.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index 1747c0b564ef..356878aabbd4 100644
--- a/include/net/af_vsock.h
+++ b/include/net/af_vsock.h
@@ -141,6 +141,7 @@ struct vsock_transport {
int (*seqpacket_enqueue)(struct vsock_sock *vsk, struct msghdr *msg,
size_t len);
bool (*seqpacket_allow)(u32 remote_cid);
+ void (*seqpacket_drop)(struct vsock_sock *vsk);
/* Notification. */
int (*notify_poll_in)(struct vsock_sock *, size_t, bool *);
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index ec54e4222cbf..27fa38090e13 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -2024,6 +2024,7 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
intr_err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);
if (intr_err <= 0) {
err = intr_err;
+ transport->seqpacket_drop(vsk);
break;
}
--
2.25.1
Set 'seqpacket_drop()' callback in transport struct.
Signed-off-by: Arseny Krasnov <[email protected]>
---
net/vmw_vsock/virtio_transport.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index e8b8108f3a29..5b9679f33baa 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -475,6 +475,7 @@ static struct virtio_transport virtio_transport = {
.seqpacket_dequeue = virtio_transport_seqpacket_dequeue,
.seqpacket_enqueue = virtio_transport_seqpacket_enqueue,
.seqpacket_allow = virtio_transport_seqpacket_allow,
+ .seqpacket_drop = virtio_transport_seqpacket_drop,
.notify_poll_in = virtio_transport_notify_poll_in,
.notify_poll_out = virtio_transport_notify_poll_out,
--
2.25.1
Set 'seqpacket_drop()' callback in transport struct.
Signed-off-by: Arseny Krasnov <[email protected]>
---
net/vmw_vsock/vsock_loopback.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c
index 809f807d0710..d9030a46e4b9 100644
--- a/net/vmw_vsock/vsock_loopback.c
+++ b/net/vmw_vsock/vsock_loopback.c
@@ -94,6 +94,7 @@ static struct virtio_transport loopback_transport = {
.seqpacket_dequeue = virtio_transport_seqpacket_dequeue,
.seqpacket_enqueue = virtio_transport_seqpacket_enqueue,
.seqpacket_allow = vsock_loopback_seqpacket_allow,
+ .seqpacket_drop = virtio_transport_seqpacket_drop,
.notify_poll_in = virtio_transport_notify_poll_in,
.notify_poll_out = virtio_transport_notify_poll_out,
--
2.25.1
Add test where sender sends two message, each with own
data pattern. Reader tries to read first to broken buffer:
it has three pages size, but middle page is unmapped. Then,
reader tries to read second message to valid buffer. Test
checks, that uncopied part of first message was dropped
and thus not copied as part of second message.
Signed-off-by: Arseny Krasnov <[email protected]>
---
tools/testing/vsock/vsock_test.c | 121 +++++++++++++++++++++++++++++++
1 file changed, 121 insertions(+)
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 67766bfe176f..697ba168e97f 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -16,6 +16,7 @@
#include <linux/kernel.h>
#include <sys/types.h>
#include <sys/socket.h>
+#include <sys/mman.h>
#include "timeout.h"
#include "control.h"
@@ -385,6 +386,121 @@ static void test_seqpacket_msg_trunc_server(const struct test_opts *opts)
close(fd);
}
+#define BUF_PATTERN_1 'a'
+#define BUF_PATTERN_2 'b'
+
+static void test_seqpacket_invalid_rec_buffer_client(const struct test_opts *opts)
+{
+ int fd;
+ unsigned char *buf1;
+ unsigned char *buf2;
+ int buf_size = getpagesize() * 3;
+
+ fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
+ if (fd < 0) {
+ perror("connect");
+ exit(EXIT_FAILURE);
+ }
+
+ buf1 = malloc(buf_size);
+ if (buf1 == NULL) {
+ perror("'malloc()' for 'buf1'");
+ exit(EXIT_FAILURE);
+ }
+
+ buf2 = malloc(buf_size);
+ if (buf2 == NULL) {
+ perror("'malloc()' for 'buf2'");
+ exit(EXIT_FAILURE);
+ }
+
+ memset(buf1, BUF_PATTERN_1, buf_size);
+ memset(buf2, BUF_PATTERN_2, buf_size);
+
+ if (send(fd, buf1, buf_size, 0) != buf_size) {
+ perror("send failed");
+ exit(EXIT_FAILURE);
+ }
+
+ if (send(fd, buf2, buf_size, 0) != buf_size) {
+ perror("send failed");
+ exit(EXIT_FAILURE);
+ }
+
+ close(fd);
+}
+
+static void test_seqpacket_invalid_rec_buffer_server(const struct test_opts *opts)
+{
+ int fd;
+ unsigned char *broken_buf;
+ unsigned char *valid_buf;
+ int page_size = getpagesize();
+ int buf_size = page_size * 3;
+ ssize_t res;
+ int prot = PROT_READ | PROT_WRITE;
+ int flags = MAP_PRIVATE | MAP_ANONYMOUS;
+ int i;
+
+ fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
+ if (fd < 0) {
+ perror("accept");
+ exit(EXIT_FAILURE);
+ }
+
+ /* Setup first buffer. */
+ broken_buf = mmap(NULL, buf_size, prot, flags, -1, 0);
+ if (broken_buf == MAP_FAILED) {
+ perror("mmap for 'broken_buf'");
+ exit(EXIT_FAILURE);
+ }
+
+ /* Unmap "hole" in buffer. */
+ if (munmap(broken_buf + page_size, page_size)) {
+ perror("'broken_buf' setup");
+ exit(EXIT_FAILURE);
+ }
+
+ valid_buf = mmap(NULL, buf_size, prot, flags, -1, 0);
+ if (valid_buf == MAP_FAILED) {
+ perror("mmap for 'valid_buf'");
+ exit(EXIT_FAILURE);
+ }
+
+ /* Try to fill buffer with unmapped middle. */
+ res = read(fd, broken_buf, buf_size);
+ if (res != -1) {
+ perror("invalid read result of 'broken_buf'");
+ exit(EXIT_FAILURE);
+ }
+
+ if (errno != ENOMEM) {
+ perror("invalid errno of 'broken_buf'");
+ exit(EXIT_FAILURE);
+ }
+
+ /* Try to fill valid buffer. */
+ res = read(fd, valid_buf, buf_size);
+ if (res != buf_size) {
+ perror("invalid read result of 'valid_buf'");
+ exit(EXIT_FAILURE);
+ }
+
+ for (i = 0; i < buf_size; i++) {
+ if (valid_buf[i] != BUF_PATTERN_2) {
+ perror("invalid pattern for valid buf");
+ exit(EXIT_FAILURE);
+ }
+ }
+
+
+ /* Unmap buffers. */
+ munmap(broken_buf, page_size);
+ munmap(broken_buf + page_size * 2, page_size);
+ munmap(valid_buf, buf_size);
+ close(fd);
+}
+
static struct test_case test_cases[] = {
{
.name = "SOCK_STREAM connection reset",
@@ -425,6 +541,11 @@ static struct test_case test_cases[] = {
.run_client = test_seqpacket_msg_trunc_client,
.run_server = test_seqpacket_msg_trunc_server,
},
+ {
+ .name = "SOCK_SEQPACKET invalid receive buffer",
+ .run_client = test_seqpacket_invalid_rec_buffer_client,
+ .run_server = test_seqpacket_invalid_rec_buffer_server,
+ },
{},
};
--
2.25.1
Set 'seqpacket_drop()' callback in transport struct.
Signed-off-by: Arseny Krasnov <[email protected]>
---
drivers/vhost/vsock.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 4118390aeab6..0c154c2ca596 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -447,6 +447,7 @@ static struct virtio_transport vhost_transport = {
.stream_rcvhiwat = virtio_transport_stream_rcvhiwat,
.stream_is_active = virtio_transport_stream_is_active,
.stream_allow = virtio_transport_stream_allow,
+ .seqpacket_drop = virtio_transport_seqpacket_drop,
.seqpacket_dequeue = virtio_transport_seqpacket_dequeue,
.seqpacket_enqueue = virtio_transport_seqpacket_enqueue,
--
2.25.1
Clean 'seqpacket_has_data()' callback in transport struct.
Signed-off-by: Arseny Krasnov <[email protected]>
---
drivers/vhost/vsock.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 119f08491d3c..4118390aeab6 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -451,7 +451,6 @@ static struct virtio_transport vhost_transport = {
.seqpacket_dequeue = virtio_transport_seqpacket_dequeue,
.seqpacket_enqueue = virtio_transport_seqpacket_enqueue,
.seqpacket_allow = vhost_transport_seqpacket_allow,
- .seqpacket_has_data = virtio_transport_seqpacket_has_data,
.notify_poll_in = virtio_transport_notify_poll_in,
.notify_poll_out = virtio_transport_notify_poll_out,
--
2.25.1
Receive "loop" now really loop: it reads fragments one by
one, sleeping if queue is empty.
NOTE: 'msg_ready' pointer is not passed to 'seqpacket_dequeue()'
here - it change callback interface, so it is moved to next patch.
Signed-off-by: Arseny Krasnov <[email protected]>
---
net/vmw_vsock/af_vsock.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 59ce35da2e5b..9552f05119f2 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -2003,6 +2003,7 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
size_t len, int flags)
{
const struct vsock_transport *transport;
+ bool msg_ready;
struct vsock_sock *vsk;
ssize_t record_len;
long timeout;
@@ -2013,23 +2014,36 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
transport = vsk->transport;
timeout = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
+ msg_ready = false;
+ record_len = 0;
- err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);
- if (err <= 0)
- goto out;
+ while (!msg_ready) {
+ ssize_t fragment_len;
+ int intr_err;
- record_len = transport->seqpacket_dequeue(vsk, msg, flags);
+ intr_err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);
+ if (intr_err <= 0) {
+ err = intr_err;
+ break;
+ }
- if (record_len < 0) {
- err = -ENOMEM;
- goto out;
+ fragment_len = transport->seqpacket_dequeue(vsk, msg, flags);
+
+ if (fragment_len < 0) {
+ err = -ENOMEM;
+ break;
+ }
+
+ record_len += fragment_len;
}
if (sk->sk_err) {
err = -sk->sk_err;
} else if (sk->sk_shutdown & RCV_SHUTDOWN) {
err = 0;
- } else {
+ }
+
+ if (msg_ready && !err) {
/* User sets MSG_TRUNC, so return real length of
* packet.
*/
@@ -2045,7 +2059,6 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
msg->msg_flags |= MSG_TRUNC;
}
-out:
return err;
}
--
2.25.1
Data will copied only if 'drop until EOR' mode is disabled, also
if EOR found, 'msg_ready' is set only if we don't have current
message to drop.
Signed-off-by: Arseny Krasnov <[email protected]>
---
include/linux/virtio_vsock.h | 2 ++
net/vmw_vsock/virtio_transport_common.c | 23 +++++++++++++++++++----
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index 7360ab7ea0af..18a50f64bf54 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h
@@ -36,6 +36,7 @@ struct virtio_vsock_sock {
u32 rx_bytes;
u32 buf_alloc;
struct list_head rx_queue;
+ bool drop_until_eor;
};
struct virtio_vsock_pkt {
@@ -89,6 +90,7 @@ virtio_transport_seqpacket_dequeue(struct vsock_sock *vsk,
struct msghdr *msg,
int flags,
bool *msg_ready);
+void virtio_transport_seqpacket_drop(struct vsock_sock *vsk);
s64 virtio_transport_stream_has_data(struct vsock_sock *vsk);
s64 virtio_transport_stream_has_space(struct vsock_sock *vsk);
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 5a46c3f94e83..a8f74cc343e4 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -425,7 +425,7 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
pkt = list_first_entry(&vvs->rx_queue, struct virtio_vsock_pkt, list);
pkt_len = (size_t)le32_to_cpu(pkt->hdr.len);
- bytes_to_copy = min(user_buf_len, pkt_len);
+ bytes_to_copy = vvs->drop_until_eor ? 0 : min(user_buf_len, pkt_len);
if (bytes_to_copy) {
int err;
@@ -438,17 +438,22 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
spin_lock_bh(&vvs->rx_lock);
- if (err)
+ if (err) {
dequeued_len = err;
- else
+ vvs->drop_until_eor = true;
+ } else {
user_buf_len -= bytes_to_copy;
+ }
}
if (dequeued_len >= 0)
dequeued_len += pkt_len;
if (le32_to_cpu(pkt->hdr.flags) & VIRTIO_VSOCK_SEQ_EOR) {
- *msg_ready = true;
+ if (vvs->drop_until_eor)
+ vvs->drop_until_eor = false;
+ else
+ *msg_ready = true;
}
virtio_transport_dec_rx_pkt(vvs, pkt);
@@ -487,6 +492,16 @@ virtio_transport_seqpacket_dequeue(struct vsock_sock *vsk,
}
EXPORT_SYMBOL_GPL(virtio_transport_seqpacket_dequeue);
+void virtio_transport_seqpacket_drop(struct vsock_sock *vsk)
+{
+ struct virtio_vsock_sock *vvs = vsk->trans;
+
+ spin_lock_bh(&vvs->rx_lock);
+ vvs->drop_until_eor = true;
+ spin_unlock_bh(&vvs->rx_lock);
+}
+EXPORT_SYMBOL_GPL(virtio_transport_seqpacket_drop);
+
int
virtio_transport_seqpacket_enqueue(struct vsock_sock *vsk,
struct msghdr *msg,
--
2.25.1
Clean 'seqpacket_has_data()' callback in transport struct.
Signed-off-by: Arseny Krasnov <[email protected]>
---
net/vmw_vsock/vsock_loopback.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/vmw_vsock/vsock_loopback.c b/net/vmw_vsock/vsock_loopback.c
index 169a8cf65b39..809f807d0710 100644
--- a/net/vmw_vsock/vsock_loopback.c
+++ b/net/vmw_vsock/vsock_loopback.c
@@ -94,7 +94,6 @@ static struct virtio_transport loopback_transport = {
.seqpacket_dequeue = virtio_transport_seqpacket_dequeue,
.seqpacket_enqueue = virtio_transport_seqpacket_enqueue,
.seqpacket_allow = vsock_loopback_seqpacket_allow,
- .seqpacket_has_data = virtio_transport_seqpacket_has_data,
.notify_poll_in = virtio_transport_notify_poll_in,
.notify_poll_out = virtio_transport_notify_poll_out,
--
2.25.1
This patch adds 'msg_ready' parameter to dequeue callback, it is
set to true if EOR is found(in case of virtio transport).
This patch contains small changes for both af_vsock and virtio
transport code to avoid build fails with partly applied patchset.
Signed-off-by: Arseny Krasnov <[email protected]>
---
include/linux/virtio_vsock.h | 3 ++-
include/net/af_vsock.h | 2 +-
net/vmw_vsock/af_vsock.c | 2 +-
net/vmw_vsock/virtio_transport_common.c | 2 +-
4 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
index 719008d4235e..8d34f3d73bbb 100644
--- a/include/linux/virtio_vsock.h
+++ b/include/linux/virtio_vsock.h
@@ -88,7 +88,8 @@ virtio_transport_seqpacket_enqueue(struct vsock_sock *vsk,
ssize_t
virtio_transport_seqpacket_dequeue(struct vsock_sock *vsk,
struct msghdr *msg,
- int flags);
+ int flags,
+ bool *msg_ready);
s64 virtio_transport_stream_has_data(struct vsock_sock *vsk);
s64 virtio_transport_stream_has_space(struct vsock_sock *vsk);
diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index bf5ea1873e6f..1747c0b564ef 100644
--- a/include/net/af_vsock.h
+++ b/include/net/af_vsock.h
@@ -137,7 +137,7 @@ struct vsock_transport {
/* SEQ_PACKET. */
ssize_t (*seqpacket_dequeue)(struct vsock_sock *vsk, struct msghdr *msg,
- int flags);
+ int flags, bool *msg_ready);
int (*seqpacket_enqueue)(struct vsock_sock *vsk, struct msghdr *msg,
size_t len);
bool (*seqpacket_allow)(u32 remote_cid);
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 9552f05119f2..ec54e4222cbf 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -2027,7 +2027,7 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
break;
}
- fragment_len = transport->seqpacket_dequeue(vsk, msg, flags);
+ fragment_len = transport->seqpacket_dequeue(vsk, msg, flags, &msg_ready);
if (fragment_len < 0) {
err = -ENOMEM;
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 319c3345f3e0..9c2bd84ab8e6 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -489,7 +489,7 @@ EXPORT_SYMBOL_GPL(virtio_transport_stream_dequeue);
ssize_t
virtio_transport_seqpacket_dequeue(struct vsock_sock *vsk,
struct msghdr *msg,
- int flags)
+ int flags, bool *msg_ready)
{
if (flags & MSG_PEEK)
return -EOPNOTSUPP;
--
2.25.1
On Mon, Jun 28, 2021 at 01:02:47PM +0300, Arseny Krasnov wrote:
>Also remove 'seqpacket_has_data' callback from transport.
>
>Signed-off-by: Arseny Krasnov <[email protected]>
>---
> include/net/af_vsock.h | 1 -
> net/vmw_vsock/af_vsock.c | 12 +-----------
> 2 files changed, 1 insertion(+), 12 deletions(-)
In order to avoid issues while bisecting the kernel, we should have
commit that doesn't break the build or the runtime, so please take this
in mind also for other commits.
For example here we removed the seqpacket_has_data callbacks assignment
before to remove where we use the callback, with a potential fault at
runtime.
I think you can simply put patches from 1 to 5 together in a single
patch.
In addition, we should move these changes after we don't need
vsock_connectible_has_data() anymore, for example, where we replace the
receive loop logic.
Thanks,
Stefano
>
>diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>index ab207677e0a8..bf5ea1873e6f 100644
>--- a/include/net/af_vsock.h
>+++ b/include/net/af_vsock.h
>@@ -141,7 +141,6 @@ struct vsock_transport {
> int (*seqpacket_enqueue)(struct vsock_sock *vsk, struct msghdr *msg,
> size_t len);
> bool (*seqpacket_allow)(u32 remote_cid);
>- u32 (*seqpacket_has_data)(struct vsock_sock *vsk);
>
> /* Notification. */
> int (*notify_poll_in)(struct vsock_sock *, size_t, bool *);
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 21ccf450e249..59ce35da2e5b 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -860,16 +860,6 @@ s64 vsock_stream_has_data(struct vsock_sock *vsk)
> }
> EXPORT_SYMBOL_GPL(vsock_stream_has_data);
>
>-static s64 vsock_connectible_has_data(struct vsock_sock *vsk)
>-{
>- struct sock *sk = sk_vsock(vsk);
>-
>- if (sk->sk_type == SOCK_SEQPACKET)
>- return vsk->transport->seqpacket_has_data(vsk);
>- else
>- return vsock_stream_has_data(vsk);
>-}
>-
> s64 vsock_stream_has_space(struct vsock_sock *vsk)
> {
> return vsk->transport->stream_has_space(vsk);
>@@ -1881,7 +1871,7 @@ static int vsock_connectible_wait_data(struct
>sock *sk,
> err = 0;
> transport = vsk->transport;
>
>- while ((data = vsock_connectible_has_data(vsk)) == 0) {
>+ while ((data = vsock_stream_has_data(vsk)) == 0) {
> prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE);
>
> if (sk->sk_err != 0 ||
>-- 2.25.1
>
On Mon, Jun 28, 2021 at 01:03:15PM +0300, Arseny Krasnov wrote:
>There is no sense to count EORs, because 'rx_bytes' is
>used to check data presence on socket.
>
>Signed-off-by: Arseny Krasnov <[email protected]>
>---
> net/vmw_vsock/virtio_transport_common.c | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 84431d7a87a5..319c3345f3e0 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1005,9 +1005,6 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
> goto out;
> }
>
>- if (le32_to_cpu(pkt->hdr.flags) & VIRTIO_VSOCK_SEQ_EOR)
>- vvs->msg_count++;
>-
Same here, please remove it when you don't need it, and also remove from
the struct virtio_vsock_sock.
Thanks,
Stefano
> /* Try to copy small packets into the buffer of last packet queued,
> * to avoid wasting memory queueing the entire buffer with a small
> * payload.
>--
>2.25.1
>
On Mon, Jun 28, 2021 at 01:03:28PM +0300, Arseny Krasnov wrote:
>Receive "loop" now really loop: it reads fragments one by
>one, sleeping if queue is empty.
>
>NOTE: 'msg_ready' pointer is not passed to 'seqpacket_dequeue()'
>here - it change callback interface, so it is moved to next patch.
>
>Signed-off-by: Arseny Krasnov <[email protected]>
>---
> net/vmw_vsock/af_vsock.c | 31 ++++++++++++++++++++++---------
> 1 file changed, 22 insertions(+), 9 deletions(-)
I think you can merge patches 8, 9, and 10 together since we
are touching the seqpacket_dequeue() behaviour.
Then you can remove in separate patches the unneeded parts (e.g.
seqpacket_has_data, msg_count, etc.).
Thanks,
Stefano
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 59ce35da2e5b..9552f05119f2 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -2003,6 +2003,7 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
> size_t len, int flags)
> {
> const struct vsock_transport *transport;
>+ bool msg_ready;
> struct vsock_sock *vsk;
> ssize_t record_len;
> long timeout;
>@@ -2013,23 +2014,36 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
> transport = vsk->transport;
>
> timeout = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>+ msg_ready = false;
>+ record_len = 0;
>
>- err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);
>- if (err <= 0)
>- goto out;
>+ while (!msg_ready) {
>+ ssize_t fragment_len;
>+ int intr_err;
>
>- record_len = transport->seqpacket_dequeue(vsk, msg, flags);
>+ intr_err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);
>+ if (intr_err <= 0) {
>+ err = intr_err;
>+ break;
>+ }
>
>- if (record_len < 0) {
>- err = -ENOMEM;
>- goto out;
>+ fragment_len = transport->seqpacket_dequeue(vsk, msg, flags);
>+
>+ if (fragment_len < 0) {
>+ err = -ENOMEM;
>+ break;
>+ }
>+
>+ record_len += fragment_len;
> }
>
> if (sk->sk_err) {
> err = -sk->sk_err;
> } else if (sk->sk_shutdown & RCV_SHUTDOWN) {
> err = 0;
>- } else {
>+ }
>+
>+ if (msg_ready && !err) {
> /* User sets MSG_TRUNC, so return real length of
> * packet.
> */
>@@ -2045,7 +2059,6 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
> msg->msg_flags |= MSG_TRUNC;
> }
>
>-out:
> return err;
> }
>
>--
>2.25.1
>
On Mon, Jun 28, 2021 at 01:04:12PM +0300, Arseny Krasnov wrote:
>Add special callback for SEQPACKET socket which is called when
>we need to drop current in-progress record: part of record was
>copied successfully, reader wait rest of record, but signal
>interrupts it and reader leaves it's loop, leaving packets of
>current record still in queue. So to avoid copy of "orphaned"
>record, we tell transport to drop every packet until EOR will
>be found.
>
>Signed-off-by: Arseny Krasnov <[email protected]>
>---
> include/net/af_vsock.h | 1 +
> net/vmw_vsock/af_vsock.c | 1 +
> 2 files changed, 2 insertions(+)
And also for this change, I think you can merge with patches 12, 13, 14,
15, otherwise if we bisect and we build at this patch, the
seqpacket_drop pointer is not valid.
Thanks,
Stefano
>
>diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>index 1747c0b564ef..356878aabbd4 100644
>--- a/include/net/af_vsock.h
>+++ b/include/net/af_vsock.h
>@@ -141,6 +141,7 @@ struct vsock_transport {
> int (*seqpacket_enqueue)(struct vsock_sock *vsk, struct msghdr *msg,
> size_t len);
> bool (*seqpacket_allow)(u32 remote_cid);
>+ void (*seqpacket_drop)(struct vsock_sock *vsk);
>
> /* Notification. */
> int (*notify_poll_in)(struct vsock_sock *, size_t, bool *);
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index ec54e4222cbf..27fa38090e13 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -2024,6 +2024,7 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
> intr_err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);
> if (intr_err <= 0) {
> err = intr_err;
>+ transport->seqpacket_drop(vsk);
> break;
> }
>
>--
>2.25.1
>
On Mon, Jun 28, 2021 at 01:05:36PM +0300, Arseny Krasnov wrote:
>Add test where sender sends two message, each with own
>data pattern. Reader tries to read first to broken buffer:
>it has three pages size, but middle page is unmapped. Then,
>reader tries to read second message to valid buffer. Test
>checks, that uncopied part of first message was dropped
>and thus not copied as part of second message.
>
>Signed-off-by: Arseny Krasnov <[email protected]>
>---
> tools/testing/vsock/vsock_test.c | 121 +++++++++++++++++++++++++++++++
> 1 file changed, 121 insertions(+)
Cool test! Thanks for doing this!
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 67766bfe176f..697ba168e97f 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -16,6 +16,7 @@
> #include <linux/kernel.h>
> #include <sys/types.h>
> #include <sys/socket.h>
>+#include <sys/mman.h>
>
> #include "timeout.h"
> #include "control.h"
>@@ -385,6 +386,121 @@ static void test_seqpacket_msg_trunc_server(const struct test_opts *opts)
> close(fd);
> }
>
>+#define BUF_PATTERN_1 'a'
>+#define BUF_PATTERN_2 'b'
>+
>+static void test_seqpacket_invalid_rec_buffer_client(const struct test_opts *opts)
>+{
>+ int fd;
>+ unsigned char *buf1;
>+ unsigned char *buf2;
>+ int buf_size = getpagesize() * 3;
>+
>+ fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
>+ if (fd < 0) {
>+ perror("connect");
>+ exit(EXIT_FAILURE);
>+ }
>+
>+ buf1 = malloc(buf_size);
>+ if (buf1 == NULL) {
checkpatch suggests to use "if (!buf1)" ...
>+ perror("'malloc()' for 'buf1'");
>+ exit(EXIT_FAILURE);
>+ }
>+
>+ buf2 = malloc(buf_size);
>+ if (buf2 == NULL) {
... and "if (!buf2)" ...
>+ perror("'malloc()' for 'buf2'");
>+ exit(EXIT_FAILURE);
>+ }
>+
>+ memset(buf1, BUF_PATTERN_1, buf_size);
>+ memset(buf2, BUF_PATTERN_2, buf_size);
>+
>+ if (send(fd, buf1, buf_size, 0) != buf_size) {
>+ perror("send failed");
>+ exit(EXIT_FAILURE);
>+ }
>+
>+ if (send(fd, buf2, buf_size, 0) != buf_size) {
>+ perror("send failed");
>+ exit(EXIT_FAILURE);
>+ }
>+
>+ close(fd);
>+}
>+
>+static void test_seqpacket_invalid_rec_buffer_server(const struct test_opts *opts)
>+{
>+ int fd;
>+ unsigned char *broken_buf;
>+ unsigned char *valid_buf;
>+ int page_size = getpagesize();
>+ int buf_size = page_size * 3;
>+ ssize_t res;
>+ int prot = PROT_READ | PROT_WRITE;
>+ int flags = MAP_PRIVATE | MAP_ANONYMOUS;
>+ int i;
>+
>+ fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
>+ if (fd < 0) {
>+ perror("accept");
>+ exit(EXIT_FAILURE);
>+ }
>+
>+ /* Setup first buffer. */
>+ broken_buf = mmap(NULL, buf_size, prot, flags, -1, 0);
>+ if (broken_buf == MAP_FAILED) {
>+ perror("mmap for 'broken_buf'");
>+ exit(EXIT_FAILURE);
>+ }
>+
>+ /* Unmap "hole" in buffer. */
>+ if (munmap(broken_buf + page_size, page_size)) {
>+ perror("'broken_buf' setup");
>+ exit(EXIT_FAILURE);
>+ }
>+
>+ valid_buf = mmap(NULL, buf_size, prot, flags, -1, 0);
>+ if (valid_buf == MAP_FAILED) {
>+ perror("mmap for 'valid_buf'");
>+ exit(EXIT_FAILURE);
>+ }
>+
>+ /* Try to fill buffer with unmapped middle. */
>+ res = read(fd, broken_buf, buf_size);
>+ if (res != -1) {
>+ perror("invalid read result of 'broken_buf'");
>+ exit(EXIT_FAILURE);
>+ }
>+
>+ if (errno != ENOMEM) {
>+ perror("invalid errno of 'broken_buf'");
>+ exit(EXIT_FAILURE);
>+ }
>+
>+ /* Try to fill valid buffer. */
>+ res = read(fd, valid_buf, buf_size);
>+ if (res != buf_size) {
>+ perror("invalid read result of 'valid_buf'");
>+ exit(EXIT_FAILURE);
>+ }
>+
>+ for (i = 0; i < buf_size; i++) {
>+ if (valid_buf[i] != BUF_PATTERN_2) {
>+ perror("invalid pattern for valid buf");
>+ exit(EXIT_FAILURE);
>+ }
>+ }
>+
>+
... and to remove the extra blank line here :-)
Thanks,
Stefano
>+ /* Unmap buffers. */
>+ munmap(broken_buf, page_size);
>+ munmap(broken_buf + page_size * 2, page_size);
>+ munmap(valid_buf, buf_size);
>+ close(fd);
>+}
>+
> static struct test_case test_cases[] = {
> {
> .name = "SOCK_STREAM connection reset",
>@@ -425,6 +541,11 @@ static struct test_case test_cases[] = {
> .run_client = test_seqpacket_msg_trunc_client,
> .run_server = test_seqpacket_msg_trunc_server,
> },
>+ {
>+ .name = "SOCK_SEQPACKET invalid receive buffer",
>+ .run_client = test_seqpacket_invalid_rec_buffer_client,
>+ .run_server = test_seqpacket_invalid_rec_buffer_server,
>+ },
> {},
> };
>
>--
>2.25.1
>
On 30.06.2021 15:11, Stefano Garzarella wrote:
> On Mon, Jun 28, 2021 at 01:03:15PM +0300, Arseny Krasnov wrote:
>> There is no sense to count EORs, because 'rx_bytes' is
>> used to check data presence on socket.
>>
>> Signed-off-by: Arseny Krasnov <[email protected]>
>> ---
>> net/vmw_vsock/virtio_transport_common.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> index 84431d7a87a5..319c3345f3e0 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -1005,9 +1005,6 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
>> goto out;
>> }
>>
>> - if (le32_to_cpu(pkt->hdr.flags) & VIRTIO_VSOCK_SEQ_EOR)
>> - vvs->msg_count++;
>> -
> Same here, please remove it when you don't need it, and also remove from
> the struct virtio_vsock_sock.
>
> Thanks,
> Stefano
Ack
>
>> /* Try to copy small packets into the buffer of last packet queued,
>> * to avoid wasting memory queueing the entire buffer with a small
>> * payload.
>> --
>> 2.25.1
>>
>
On 30.06.2021 15:12, Stefano Garzarella wrote:
> On Mon, Jun 28, 2021 at 01:03:28PM +0300, Arseny Krasnov wrote:
>> Receive "loop" now really loop: it reads fragments one by
>> one, sleeping if queue is empty.
>>
>> NOTE: 'msg_ready' pointer is not passed to 'seqpacket_dequeue()'
>> here - it change callback interface, so it is moved to next patch.
>>
>> Signed-off-by: Arseny Krasnov <[email protected]>
>> ---
>> net/vmw_vsock/af_vsock.c | 31 ++++++++++++++++++++++---------
>> 1 file changed, 22 insertions(+), 9 deletions(-)
> I think you can merge patches 8, 9, and 10 together since we
> are touching the seqpacket_dequeue() behaviour.
>
> Then you can remove in separate patches the unneeded parts (e.g.
> seqpacket_has_data, msg_count, etc.).
>
> Thanks,
> Stefano
Ack
>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index 59ce35da2e5b..9552f05119f2 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -2003,6 +2003,7 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
>> size_t len, int flags)
>> {
>> const struct vsock_transport *transport;
>> + bool msg_ready;
>> struct vsock_sock *vsk;
>> ssize_t record_len;
>> long timeout;
>> @@ -2013,23 +2014,36 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
>> transport = vsk->transport;
>>
>> timeout = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>> + msg_ready = false;
>> + record_len = 0;
>>
>> - err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);
>> - if (err <= 0)
>> - goto out;
>> + while (!msg_ready) {
>> + ssize_t fragment_len;
>> + int intr_err;
>>
>> - record_len = transport->seqpacket_dequeue(vsk, msg, flags);
>> + intr_err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);
>> + if (intr_err <= 0) {
>> + err = intr_err;
>> + break;
>> + }
>>
>> - if (record_len < 0) {
>> - err = -ENOMEM;
>> - goto out;
>> + fragment_len = transport->seqpacket_dequeue(vsk, msg, flags);
>> +
>> + if (fragment_len < 0) {
>> + err = -ENOMEM;
>> + break;
>> + }
>> +
>> + record_len += fragment_len;
>> }
>>
>> if (sk->sk_err) {
>> err = -sk->sk_err;
>> } else if (sk->sk_shutdown & RCV_SHUTDOWN) {
>> err = 0;
>> - } else {
>> + }
>> +
>> + if (msg_ready && !err) {
>> /* User sets MSG_TRUNC, so return real length of
>> * packet.
>> */
>> @@ -2045,7 +2059,6 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
>> msg->msg_flags |= MSG_TRUNC;
>> }
>>
>> -out:
>> return err;
>> }
>>
>> --
>> 2.25.1
>>
>
On 30.06.2021 15:10, Stefano Garzarella wrote:
> On Mon, Jun 28, 2021 at 01:02:47PM +0300, Arseny Krasnov wrote:
>> Also remove 'seqpacket_has_data' callback from transport.
>>
>> Signed-off-by: Arseny Krasnov <[email protected]>
>> ---
>> include/net/af_vsock.h | 1 -
>> net/vmw_vsock/af_vsock.c | 12 +-----------
>> 2 files changed, 1 insertion(+), 12 deletions(-)
> In order to avoid issues while bisecting the kernel, we should have
> commit that doesn't break the build or the runtime, so please take this
> in mind also for other commits.
>
> For example here we removed the seqpacket_has_data callbacks assignment
> before to remove where we use the callback, with a potential fault at
> runtime.
>
> I think you can simply put patches from 1 to 5 together in a single
> patch.
>
> In addition, we should move these changes after we don't need
> vsock_connectible_has_data() anymore, for example, where we replace the
> receive loop logic.
>
> Thanks,
> Stefano
Ack
>
>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>> index ab207677e0a8..bf5ea1873e6f 100644
>> --- a/include/net/af_vsock.h
>> +++ b/include/net/af_vsock.h
>> @@ -141,7 +141,6 @@ struct vsock_transport {
>> int (*seqpacket_enqueue)(struct vsock_sock *vsk, struct msghdr *msg,
>> size_t len);
>> bool (*seqpacket_allow)(u32 remote_cid);
>> - u32 (*seqpacket_has_data)(struct vsock_sock *vsk);
>>
>> /* Notification. */
>> int (*notify_poll_in)(struct vsock_sock *, size_t, bool *);
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index 21ccf450e249..59ce35da2e5b 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -860,16 +860,6 @@ s64 vsock_stream_has_data(struct vsock_sock *vsk)
>> }
>> EXPORT_SYMBOL_GPL(vsock_stream_has_data);
>>
>> -static s64 vsock_connectible_has_data(struct vsock_sock *vsk)
>> -{
>> - struct sock *sk = sk_vsock(vsk);
>> -
>> - if (sk->sk_type == SOCK_SEQPACKET)
>> - return vsk->transport->seqpacket_has_data(vsk);
>> - else
>> - return vsock_stream_has_data(vsk);
>> -}
>> -
>> s64 vsock_stream_has_space(struct vsock_sock *vsk)
>> {
>> return vsk->transport->stream_has_space(vsk);
>> @@ -1881,7 +1871,7 @@ static int vsock_connectible_wait_data(struct
>> sock *sk,
>> err = 0;
>> transport = vsk->transport;
>>
>> - while ((data = vsock_connectible_has_data(vsk)) == 0) {
>> + while ((data = vsock_stream_has_data(vsk)) == 0) {
>> prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE);
>>
>> if (sk->sk_err != 0 ||
>> -- 2.25.1
>>
>
On 30.06.2021 15:12, Stefano Garzarella wrote:
> On Mon, Jun 28, 2021 at 01:04:12PM +0300, Arseny Krasnov wrote:
>> Add special callback for SEQPACKET socket which is called when
>> we need to drop current in-progress record: part of record was
>> copied successfully, reader wait rest of record, but signal
>> interrupts it and reader leaves it's loop, leaving packets of
>> current record still in queue. So to avoid copy of "orphaned"
>> record, we tell transport to drop every packet until EOR will
>> be found.
>>
>> Signed-off-by: Arseny Krasnov <[email protected]>
>> ---
>> include/net/af_vsock.h | 1 +
>> net/vmw_vsock/af_vsock.c | 1 +
>> 2 files changed, 2 insertions(+)
> And also for this change, I think you can merge with patches 12, 13, 14,
> 15, otherwise if we bisect and we build at this patch, the
> seqpacket_drop pointer is not valid.
>
> Thanks,
> Stefano
Ack
>
>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>> index 1747c0b564ef..356878aabbd4 100644
>> --- a/include/net/af_vsock.h
>> +++ b/include/net/af_vsock.h
>> @@ -141,6 +141,7 @@ struct vsock_transport {
>> int (*seqpacket_enqueue)(struct vsock_sock *vsk, struct msghdr *msg,
>> size_t len);
>> bool (*seqpacket_allow)(u32 remote_cid);
>> + void (*seqpacket_drop)(struct vsock_sock *vsk);
>>
>> /* Notification. */
>> int (*notify_poll_in)(struct vsock_sock *, size_t, bool *);
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index ec54e4222cbf..27fa38090e13 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -2024,6 +2024,7 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk, struct msghdr *msg,
>> intr_err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);
>> if (intr_err <= 0) {
>> err = intr_err;
>> + transport->seqpacket_drop(vsk);
>> break;
>> }
>>
>> --
>> 2.25.1
>>
>