linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic
@ 2021-06-28  9:59 Arseny Krasnov
  2021-06-28 10:01 ` [RFC PATCH v1 01/16] vhost/vsock: don't set 'seqpacket_has_data()' callback Arseny Krasnov
                   ` (15 more replies)
  0 siblings, 16 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28  9:59 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Colin Ian King, Andra Paraschiv, Norbert Slusarek
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

	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 <arseny.krasnov@kaspersky.com>

-- 
2.25.1


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 01/16] vhost/vsock: don't set 'seqpacket_has_data()' callback
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
@ 2021-06-28 10:01 ` Arseny Krasnov
  2021-06-28 10:01 ` [RFC PATCH v1 02/16] vsock/loopback: " Arseny Krasnov
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:01 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Colin Ian King, Andra Paraschiv, Norbert Slusarek
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

Clean 'seqpacket_has_data()' callback in transport struct.

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 02/16] vsock/loopback: don't set 'seqpacket_has_data()' callback
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
  2021-06-28 10:01 ` [RFC PATCH v1 01/16] vhost/vsock: don't set 'seqpacket_has_data()' callback Arseny Krasnov
@ 2021-06-28 10:01 ` Arseny Krasnov
  2021-06-28 10:02 ` [RFC PATCH v1 03/16] virtio/vsock: " Arseny Krasnov
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:01 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Colin Ian King, Norbert Slusarek, Andra Paraschiv
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

Clean 'seqpacket_has_data()' callback in transport struct.

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 03/16] virtio/vsock: don't set 'seqpacket_has_data()' callback
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
  2021-06-28 10:01 ` [RFC PATCH v1 01/16] vhost/vsock: don't set 'seqpacket_has_data()' callback Arseny Krasnov
  2021-06-28 10:01 ` [RFC PATCH v1 02/16] vsock/loopback: " Arseny Krasnov
@ 2021-06-28 10:02 ` Arseny Krasnov
  2021-06-28 10:02 ` [RFC PATCH v1 04/16] virtio/vsock: remove 'virtio_transport_seqpacket_has_data' Arseny Krasnov
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:02 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Andra Paraschiv, Colin Ian King, Norbert Slusarek
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

Clean 'seqpacket_has_data()' callback in transport struct.

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 04/16] virtio/vsock: remove 'virtio_transport_seqpacket_has_data'
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (2 preceding siblings ...)
  2021-06-28 10:02 ` [RFC PATCH v1 03/16] virtio/vsock: " Arseny Krasnov
@ 2021-06-28 10:02 ` Arseny Krasnov
  2021-06-28 10:02 ` [RFC PATCH v1 05/16] af_vsock: use SOCK_STREAM function to check data Arseny Krasnov
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:02 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Colin Ian King, Norbert Slusarek, Andra Paraschiv
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

As now 'rx_bytes' is used to check presence of data on socket,
this function is obsolete.

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 05/16] af_vsock: use SOCK_STREAM function to check data
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (3 preceding siblings ...)
  2021-06-28 10:02 ` [RFC PATCH v1 04/16] virtio/vsock: remove 'virtio_transport_seqpacket_has_data' Arseny Krasnov
@ 2021-06-28 10:02 ` Arseny Krasnov
  2021-06-30 12:10   ` Stefano Garzarella
  2021-06-28 10:03 ` [RFC PATCH v1 06/16] vsock/virtio: remove record size limit for SEQPACKET Arseny Krasnov
                   ` (10 subsequent siblings)
  15 siblings, 1 reply; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:02 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Norbert Slusarek, Colin Ian King, Andra Paraschiv
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

Also remove 'seqpacket_has_data' callback from transport.

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 06/16] vsock/virtio: remove record size limit for SEQPACKET
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (4 preceding siblings ...)
  2021-06-28 10:02 ` [RFC PATCH v1 05/16] af_vsock: use SOCK_STREAM function to check data Arseny Krasnov
@ 2021-06-28 10:03 ` Arseny Krasnov
  2021-06-28 10:03 ` [RFC PATCH v1 07/16] virtio/vsock: don't count EORs on receive Arseny Krasnov
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:03 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Andra Paraschiv, Colin Ian King, Norbert Slusarek
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

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 <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 07/16] virtio/vsock: don't count EORs on receive
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (5 preceding siblings ...)
  2021-06-28 10:03 ` [RFC PATCH v1 06/16] vsock/virtio: remove record size limit for SEQPACKET Arseny Krasnov
@ 2021-06-28 10:03 ` Arseny Krasnov
  2021-06-30 12:11   ` Stefano Garzarella
  2021-06-28 10:03 ` [RFC PATCH v1 08/16] af_vsock: change SEQPACKET receive loop Arseny Krasnov
                   ` (8 subsequent siblings)
  15 siblings, 1 reply; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:03 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Norbert Slusarek, Andra Paraschiv, Colin Ian King
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

There is no sense to count EORs, because 'rx_bytes' is
used to check data presence on socket.

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 08/16] af_vsock: change SEQPACKET receive loop
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (6 preceding siblings ...)
  2021-06-28 10:03 ` [RFC PATCH v1 07/16] virtio/vsock: don't count EORs on receive Arseny Krasnov
@ 2021-06-28 10:03 ` Arseny Krasnov
  2021-06-30 12:12   ` Stefano Garzarella
  2021-06-28 10:03 ` [RFC PATCH v1 09/16] af_vsock/virtio: update dequeue callback interface Arseny Krasnov
                   ` (7 subsequent siblings)
  15 siblings, 1 reply; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:03 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Jorgen Hansen, Andra Paraschiv, Colin Ian King, Norbert Slusarek
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

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 <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 09/16] af_vsock/virtio: update dequeue callback interface
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (7 preceding siblings ...)
  2021-06-28 10:03 ` [RFC PATCH v1 08/16] af_vsock: change SEQPACKET receive loop Arseny Krasnov
@ 2021-06-28 10:03 ` Arseny Krasnov
  2021-06-28 10:03 ` [RFC PATCH v1 10/16] virtio/vsock: update SEQPACKET dequeue logic Arseny Krasnov
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:03 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Jorgen Hansen, Andra Paraschiv, Colin Ian King, Norbert Slusarek
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

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 <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 10/16] virtio/vsock: update SEQPACKET dequeue logic
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (8 preceding siblings ...)
  2021-06-28 10:03 ` [RFC PATCH v1 09/16] af_vsock/virtio: update dequeue callback interface Arseny Krasnov
@ 2021-06-28 10:03 ` Arseny Krasnov
  2021-06-28 10:04 ` [RFC PATCH v1 11/16] afvsock: add 'seqpacket_drop()' Arseny Krasnov
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:03 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Colin Ian King, Norbert Slusarek, Andra Paraschiv
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

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 <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 11/16] afvsock: add 'seqpacket_drop()'
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (9 preceding siblings ...)
  2021-06-28 10:03 ` [RFC PATCH v1 10/16] virtio/vsock: update SEQPACKET dequeue logic Arseny Krasnov
@ 2021-06-28 10:04 ` Arseny Krasnov
  2021-06-30 12:12   ` Stefano Garzarella
  2021-06-28 10:04 ` [RFC PATCH v1 12/16] virtio/vsock: add 'drop until EOR' logic Arseny Krasnov
                   ` (4 subsequent siblings)
  15 siblings, 1 reply; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:04 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Colin Ian King, Norbert Slusarek, Andra Paraschiv
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

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 <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 12/16] virtio/vsock: add 'drop until EOR' logic
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (10 preceding siblings ...)
  2021-06-28 10:04 ` [RFC PATCH v1 11/16] afvsock: add 'seqpacket_drop()' Arseny Krasnov
@ 2021-06-28 10:04 ` Arseny Krasnov
  2021-06-28 10:04 ` [RFC PATCH v1 13/16] vhost/vsock: enable 'seqpacket_drop' callback in transport Arseny Krasnov
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:04 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Andra Paraschiv, Norbert Slusarek, Colin Ian King
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

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 <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 13/16] vhost/vsock: enable 'seqpacket_drop' callback in transport
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (11 preceding siblings ...)
  2021-06-28 10:04 ` [RFC PATCH v1 12/16] virtio/vsock: add 'drop until EOR' logic Arseny Krasnov
@ 2021-06-28 10:04 ` Arseny Krasnov
  2021-06-28 10:05 ` [RFC PATCH v1 14/16] virtio/vsock: " Arseny Krasnov
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:04 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Jorgen Hansen, Colin Ian King, Norbert Slusarek, Andra Paraschiv
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

Set 'seqpacket_drop()' callback in transport struct.

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 14/16] virtio/vsock: enable 'seqpacket_drop' callback in transport
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (12 preceding siblings ...)
  2021-06-28 10:04 ` [RFC PATCH v1 13/16] vhost/vsock: enable 'seqpacket_drop' callback in transport Arseny Krasnov
@ 2021-06-28 10:05 ` Arseny Krasnov
  2021-06-28 10:05 ` [RFC PATCH v1 15/16] vsock/loopback: " Arseny Krasnov
  2021-06-28 10:05 ` [RFC PATCH v1 16/16] vsock_test: SEQPACKET read to broken buffer Arseny Krasnov
  15 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:05 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Colin Ian King, Norbert Slusarek, Andra Paraschiv
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

Set 'seqpacket_drop()' callback in transport struct.

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 15/16] vsock/loopback: enable 'seqpacket_drop' callback in transport
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (13 preceding siblings ...)
  2021-06-28 10:05 ` [RFC PATCH v1 14/16] virtio/vsock: " Arseny Krasnov
@ 2021-06-28 10:05 ` Arseny Krasnov
  2021-06-28 10:05 ` [RFC PATCH v1 16/16] vsock_test: SEQPACKET read to broken buffer Arseny Krasnov
  15 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:05 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Norbert Slusarek, Andra Paraschiv, Colin Ian King
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

Set 'seqpacket_drop()' callback in transport struct.

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [RFC PATCH v1 16/16] vsock_test: SEQPACKET read to broken buffer
  2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
                   ` (14 preceding siblings ...)
  2021-06-28 10:05 ` [RFC PATCH v1 15/16] vsock/loopback: " Arseny Krasnov
@ 2021-06-28 10:05 ` Arseny Krasnov
  2021-06-30 12:17   ` Stefano Garzarella
  15 siblings, 1 reply; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-28 10:05 UTC (permalink / raw)
  To: Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Arseny Krasnov,
	Jorgen Hansen, Norbert Slusarek, Colin Ian King, Andra Paraschiv
  Cc: kvm, virtualization, netdev, linux-kernel, oxffffaa

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 <arseny.krasnov@kaspersky.com>
---
 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


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v1 05/16] af_vsock: use SOCK_STREAM function to check data
  2021-06-28 10:02 ` [RFC PATCH v1 05/16] af_vsock: use SOCK_STREAM function to check data Arseny Krasnov
@ 2021-06-30 12:10   ` Stefano Garzarella
  2021-06-30 17:47     ` [MASSMAIL KLMS] " Arseny Krasnov
  0 siblings, 1 reply; 26+ messages in thread
From: Stefano Garzarella @ 2021-06-30 12:10 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: Stefan Hajnoczi, Michael S. Tsirkin, Jason Wang, David S. Miller,
	Jakub Kicinski, Norbert Slusarek, Colin Ian King,
	Andra Paraschiv, kvm, Linux Virtualization, netdev, kernel list,
	Krasnov Arseniy

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 <arseny.krasnov@kaspersky.com>
>---
> 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
>


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v1 07/16] virtio/vsock: don't count EORs on receive
  2021-06-28 10:03 ` [RFC PATCH v1 07/16] virtio/vsock: don't count EORs on receive Arseny Krasnov
@ 2021-06-30 12:11   ` Stefano Garzarella
  2021-06-30 17:47     ` Arseny Krasnov
  0 siblings, 1 reply; 26+ messages in thread
From: Stefano Garzarella @ 2021-06-30 12:11 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: Stefan Hajnoczi, Michael S. Tsirkin, Jason Wang, David S. Miller,
	Jakub Kicinski, Norbert Slusarek, Andra Paraschiv,
	Colin Ian King, kvm, Linux Virtualization, netdev, kernel list,
	Krasnov Arseniy

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 <arseny.krasnov@kaspersky.com>
>---
> 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
>


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v1 08/16] af_vsock: change SEQPACKET receive loop
  2021-06-28 10:03 ` [RFC PATCH v1 08/16] af_vsock: change SEQPACKET receive loop Arseny Krasnov
@ 2021-06-30 12:12   ` Stefano Garzarella
  2021-06-30 17:47     ` Arseny Krasnov
  0 siblings, 1 reply; 26+ messages in thread
From: Stefano Garzarella @ 2021-06-30 12:12 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: Stefan Hajnoczi, Michael S. Tsirkin, Jason Wang, David S. Miller,
	Jakub Kicinski, Jorgen Hansen, Andra Paraschiv, Colin Ian King,
	Norbert Slusarek, kvm, Linux Virtualization, netdev, kernel list,
	Krasnov Arseniy

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 <arseny.krasnov@kaspersky.com>
>---
> 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
>


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v1 11/16] afvsock: add 'seqpacket_drop()'
  2021-06-28 10:04 ` [RFC PATCH v1 11/16] afvsock: add 'seqpacket_drop()' Arseny Krasnov
@ 2021-06-30 12:12   ` Stefano Garzarella
  2021-06-30 17:48     ` Arseny Krasnov
  0 siblings, 1 reply; 26+ messages in thread
From: Stefano Garzarella @ 2021-06-30 12:12 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: Stefan Hajnoczi, Michael S. Tsirkin, Jason Wang, David S. Miller,
	Jakub Kicinski, Colin Ian King, Norbert Slusarek,
	Andra Paraschiv, kvm, Linux Virtualization, netdev, kernel list,
	Krasnov Arseniy

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 <arseny.krasnov@kaspersky.com>
>---
> 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
>


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v1 16/16] vsock_test: SEQPACKET read to broken buffer
  2021-06-28 10:05 ` [RFC PATCH v1 16/16] vsock_test: SEQPACKET read to broken buffer Arseny Krasnov
@ 2021-06-30 12:17   ` Stefano Garzarella
  0 siblings, 0 replies; 26+ messages in thread
From: Stefano Garzarella @ 2021-06-30 12:17 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: Stefan Hajnoczi, Michael S. Tsirkin, Jason Wang, David S. Miller,
	Jakub Kicinski, Jorgen Hansen, Norbert Slusarek, Colin Ian King,
	Andra Paraschiv, kvm, Linux Virtualization, netdev, kernel list,
	Krasnov Arseniy

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 <arseny.krasnov@kaspersky.com>
>---
> 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
>


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [MASSMAIL KLMS] Re: [RFC PATCH v1 05/16] af_vsock: use SOCK_STREAM function to check data
  2021-06-30 12:10   ` Stefano Garzarella
@ 2021-06-30 17:47     ` Arseny Krasnov
  0 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-30 17:47 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Stefan Hajnoczi, Michael S. Tsirkin, Jason Wang, David S. Miller,
	Jakub Kicinski, Norbert Slusarek, Colin Ian King,
	Andra Paraschiv, kvm, Linux Virtualization, netdev, kernel list,
	Krasnov Arseniy


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 <arseny.krasnov@kaspersky.com>
>> ---
>> 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
>>
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v1 07/16] virtio/vsock: don't count EORs on receive
  2021-06-30 12:11   ` Stefano Garzarella
@ 2021-06-30 17:47     ` Arseny Krasnov
  0 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-30 17:47 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Stefan Hajnoczi, Michael S. Tsirkin, Jason Wang, David S. Miller,
	Jakub Kicinski, Norbert Slusarek, Andra Paraschiv,
	Colin Ian King, kvm, Linux Virtualization, netdev, kernel list,
	Krasnov Arseniy


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 <arseny.krasnov@kaspersky.com>
>> ---
>> 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
>>
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v1 08/16] af_vsock: change SEQPACKET receive loop
  2021-06-30 12:12   ` Stefano Garzarella
@ 2021-06-30 17:47     ` Arseny Krasnov
  0 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-30 17:47 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Stefan Hajnoczi, Michael S. Tsirkin, Jason Wang, David S. Miller,
	Jakub Kicinski, Jorgen Hansen, Andra Paraschiv, Colin Ian King,
	Norbert Slusarek, kvm, Linux Virtualization, netdev, kernel list,
	Krasnov Arseniy


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 <arseny.krasnov@kaspersky.com>
>> ---
>> 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
>>
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [RFC PATCH v1 11/16] afvsock: add 'seqpacket_drop()'
  2021-06-30 12:12   ` Stefano Garzarella
@ 2021-06-30 17:48     ` Arseny Krasnov
  0 siblings, 0 replies; 26+ messages in thread
From: Arseny Krasnov @ 2021-06-30 17:48 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Stefan Hajnoczi, Michael S. Tsirkin, Jason Wang, David S. Miller,
	Jakub Kicinski, Colin Ian King, Norbert Slusarek,
	Andra Paraschiv, kvm, Linux Virtualization, netdev, kernel list,
	Krasnov Arseniy


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 <arseny.krasnov@kaspersky.com>
>> ---
>> 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
>>
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2021-06-30 17:48 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-28  9:59 [RFC PATCH v1 00/16] Improve SOCK_SEQPACKET receive logic Arseny Krasnov
2021-06-28 10:01 ` [RFC PATCH v1 01/16] vhost/vsock: don't set 'seqpacket_has_data()' callback Arseny Krasnov
2021-06-28 10:01 ` [RFC PATCH v1 02/16] vsock/loopback: " Arseny Krasnov
2021-06-28 10:02 ` [RFC PATCH v1 03/16] virtio/vsock: " Arseny Krasnov
2021-06-28 10:02 ` [RFC PATCH v1 04/16] virtio/vsock: remove 'virtio_transport_seqpacket_has_data' Arseny Krasnov
2021-06-28 10:02 ` [RFC PATCH v1 05/16] af_vsock: use SOCK_STREAM function to check data Arseny Krasnov
2021-06-30 12:10   ` Stefano Garzarella
2021-06-30 17:47     ` [MASSMAIL KLMS] " Arseny Krasnov
2021-06-28 10:03 ` [RFC PATCH v1 06/16] vsock/virtio: remove record size limit for SEQPACKET Arseny Krasnov
2021-06-28 10:03 ` [RFC PATCH v1 07/16] virtio/vsock: don't count EORs on receive Arseny Krasnov
2021-06-30 12:11   ` Stefano Garzarella
2021-06-30 17:47     ` Arseny Krasnov
2021-06-28 10:03 ` [RFC PATCH v1 08/16] af_vsock: change SEQPACKET receive loop Arseny Krasnov
2021-06-30 12:12   ` Stefano Garzarella
2021-06-30 17:47     ` Arseny Krasnov
2021-06-28 10:03 ` [RFC PATCH v1 09/16] af_vsock/virtio: update dequeue callback interface Arseny Krasnov
2021-06-28 10:03 ` [RFC PATCH v1 10/16] virtio/vsock: update SEQPACKET dequeue logic Arseny Krasnov
2021-06-28 10:04 ` [RFC PATCH v1 11/16] afvsock: add 'seqpacket_drop()' Arseny Krasnov
2021-06-30 12:12   ` Stefano Garzarella
2021-06-30 17:48     ` Arseny Krasnov
2021-06-28 10:04 ` [RFC PATCH v1 12/16] virtio/vsock: add 'drop until EOR' logic Arseny Krasnov
2021-06-28 10:04 ` [RFC PATCH v1 13/16] vhost/vsock: enable 'seqpacket_drop' callback in transport Arseny Krasnov
2021-06-28 10:05 ` [RFC PATCH v1 14/16] virtio/vsock: " Arseny Krasnov
2021-06-28 10:05 ` [RFC PATCH v1 15/16] vsock/loopback: " Arseny Krasnov
2021-06-28 10:05 ` [RFC PATCH v1 16/16] vsock_test: SEQPACKET read to broken buffer Arseny Krasnov
2021-06-30 12:17   ` Stefano Garzarella

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).