[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-virtualization
Subject: Re: [RFC PATCH v3 09/13] virtio/vsock: add SEQPACKET receive logic
From: Stefano Garzarella <sgarzare () redhat ! com>
Date: 2021-01-29 10:55:46
Message-ID: 20210129105546.ldose6xjjjh3crye () steredhat
[Download RAW message or body]
On Mon, Jan 25, 2021 at 02:14:41PM +0300, Arseny Krasnov wrote:
>This modifies current receive logic for SEQPACKET support:
>1) Inserts 'SEQ_BEGIN' packet to socket's rx queue.
>2) Inserts 'RW' packet to socket's rx queue, but without merging with
> buffer of last packet in queue.
>3) Performs check for packet and socket types on receive(if mismatch,
> then reset connection).
>
>Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
>---
> net/vmw_vsock/virtio_transport_common.c | 79 ++++++++++++++++++-------
> 1 file changed, 58 insertions(+), 21 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index dcce35d7b462..90f9feef9d8f 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -397,6 +397,14 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
> return err;
> }
>
>+static u16 virtio_transport_get_type(struct sock *sk)
>+{
>+ if (sk->sk_type == SOCK_STREAM)
>+ return VIRTIO_VSOCK_TYPE_STREAM;
>+ else
>+ return VIRTIO_VSOCK_TYPE_SEQPACKET;
>+}
>+
> static inline void virtio_transport_del_n_free_pkt(struct virtio_vsock_pkt *pkt)
> {
> list_del(&pkt->list);
>@@ -1050,39 +1058,49 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
> struct virtio_vsock_pkt *pkt)
> {
> struct virtio_vsock_sock *vvs = vsk->trans;
>- bool can_enqueue, free_pkt = false;
>+ bool free_pkt = false;
>
> pkt->len = le32_to_cpu(pkt->hdr.len);
> pkt->off = 0;
>
> spin_lock_bh(&vvs->rx_lock);
>
>- can_enqueue = virtio_transport_inc_rx_pkt(vvs, pkt);
>- if (!can_enqueue) {
>+ if (!virtio_transport_inc_rx_pkt(vvs, pkt)) {
This is an unnecessary change, it should go in another patch.
> free_pkt = true;
> goto out;
> }
>
>- /* Try to copy small packets into the buffer of last packet queued,
>- * to avoid wasting memory queueing the entire buffer with a small
>- * payload.
>- */
>- if (pkt->len <= GOOD_COPY_LEN && !list_empty(&vvs->rx_queue)) {
>- struct virtio_vsock_pkt *last_pkt;
>+ switch (le16_to_cpu(pkt->hdr.type)) {
>+ case VIRTIO_VSOCK_TYPE_STREAM: {
We already checked the 'type' in virtio_transport_recv_pkt(), so there
is no need to this switch, I think you can simply add
if (le16_to_cpu(pkt->hdr.type) == VIRTIO_VSOCK_TYPE_STREAM) {
>+ /* Try to copy small packets into the buffer of last packet queued,
>+ * to avoid wasting memory queueing the entire buffer with a small
>+ * payload.
>+ */
>+ if (pkt->len <= GOOD_COPY_LEN && !list_empty(&vvs->rx_queue)) {
>+ struct virtio_vsock_pkt *last_pkt;
>
>- last_pkt = list_last_entry(&vvs->rx_queue,
>- struct virtio_vsock_pkt, list);
>+ last_pkt = list_last_entry(&vvs->rx_queue,
>+ struct virtio_vsock_pkt, list);
>
>- /* If there is space in the last packet queued, we copy the
>- * new packet in its buffer.
>- */
>- if (pkt->len <= last_pkt->buf_len - last_pkt->len) {
>- memcpy(last_pkt->buf + last_pkt->len, pkt->buf,
>- pkt->len);
>- last_pkt->len += pkt->len;
>- free_pkt = true;
>- goto out;
>+ /* If there is space in the last packet queued, we copy the
>+ * new packet in its buffer.
>+ */
>+ if (pkt->len <= last_pkt->buf_len - last_pkt->len) {
>+ memcpy(last_pkt->buf + last_pkt->len, pkt->buf,
>+ pkt->len);
>+ last_pkt->len += pkt->len;
>+ free_pkt = true;
>+ goto out;
>+ }
> }
>+
>+ break;
>+ }
>+ case VIRTIO_VSOCK_TYPE_SEQPACKET: {
>+ break;
>+ }
>+ default:
>+ goto out;
> }
>
> list_add_tail(&pkt->list, &vvs->rx_queue);
>@@ -1101,6 +1119,14 @@ virtio_transport_recv_connected(struct sock *sk,
> int err = 0;
>
> switch (le16_to_cpu(pkt->hdr.op)) {
>+ case VIRTIO_VSOCK_OP_SEQ_BEGIN: {
>+ struct virtio_vsock_sock *vvs = vsk->trans;
>+
>+ spin_lock_bh(&vvs->rx_lock);
>+ list_add_tail(&pkt->list, &vvs->rx_queue);
>+ spin_unlock_bh(&vvs->rx_lock);
>+ return err;
>+ }
Are there any problems if we use virtio_transport_recv_enqueue() in this
case?
> case VIRTIO_VSOCK_OP_RW:
> virtio_transport_recv_enqueue(vsk, pkt);
> sk->sk_data_ready(sk);
>@@ -1247,6 +1273,12 @@ virtio_transport_recv_listen(struct sock *sk, struct virtio_vsock_pkt *pkt,
> return 0;
> }
>
>+static bool virtio_transport_valid_type(u16 type)
>+{
>+ return (type == VIRTIO_VSOCK_TYPE_STREAM) ||
>+ (type == VIRTIO_VSOCK_TYPE_SEQPACKET);
>+}
>+
> /* We are under the virtio-vsock's vsock->rx_lock or vhost-vsock's vq->mutex
> * lock.
> */
>@@ -1272,7 +1304,7 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
> le32_to_cpu(pkt->hdr.buf_alloc),
> le32_to_cpu(pkt->hdr.fwd_cnt));
>
>- if (le16_to_cpu(pkt->hdr.type) != VIRTIO_VSOCK_TYPE_STREAM) {
>+ if (!virtio_transport_valid_type(le16_to_cpu(pkt->hdr.type))) {
> (void)virtio_transport_reset_no_sock(t, pkt);
> goto free_pkt;
> }
>@@ -1289,6 +1321,11 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
> }
> }
>
>+ if (virtio_transport_get_type(sk) != le16_to_cpu(pkt->hdr.type)) {
>+ (void)virtio_transport_reset_no_sock(t, pkt);
>+ goto free_pkt;
>+ }
>+
> vsk = vsock_sk(sk);
>
> space_available = virtio_transport_space_update(sk, pkt);
>--
>2.25.1
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic