Hi Dave,
talking to Michael, we realized that this patch merged upstream (commit
6dbd3e66e7785a2f055bf84d98de9b8fd31ff3f5) solves an issue in the
device emulation in the vhost-vsock module, because the emulation
did not meet the specification, assuming that the buffer in the RX
virtqueue was always 4 KB, without checking the actual size.
We think it's better to apply this patch in -stable.
What do you think?
Thanks,
Stefano
On Tue, Jul 30, 2019 at 5:44 PM Stefano Garzarella <sgarzare@xxxxxxxxxx> wrote:
>
> If the packets to sent to the guest are bigger than the buffer
> available, we can split them, using multiple buffers and fixing
> the length in the packet header.
> This is safe since virtio-vsock supports only stream sockets.
>
> Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> Reviewed-by: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
> Acked-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> ---
> drivers/vhost/vsock.c | 66 ++++++++++++++++++-------
> net/vmw_vsock/virtio_transport_common.c | 15 ++++--
> 2 files changed, 60 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 6c8390a2af52..9f57736fe15e 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -102,7 +102,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> struct iov_iter iov_iter;
> unsigned out, in;
> size_t nbytes;
> - size_t len;
> + size_t iov_len, payload_len;
> int head;
>
> spin_lock_bh(&vsock->send_pkt_list_lock);
> @@ -147,8 +147,24 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> break;
> }
>
> - len = iov_length(&vq->iov[out], in);
> - iov_iter_init(&iov_iter, READ, &vq->iov[out], in, len);
> + iov_len = iov_length(&vq->iov[out], in);
> + if (iov_len < sizeof(pkt->hdr)) {
> + virtio_transport_free_pkt(pkt);
> + vq_err(vq, "Buffer len [%zu] too small\n", iov_len);
> + break;
> + }
> +
> + iov_iter_init(&iov_iter, READ, &vq->iov[out], in, iov_len);
> + payload_len = pkt->len - pkt->off;
> +
> + /* If the packet is greater than the space available in the
> + * buffer, we split it using multiple buffers.
> + */
> + if (payload_len > iov_len - sizeof(pkt->hdr))
> + payload_len = iov_len - sizeof(pkt->hdr);
> +
> + /* Set the correct length in the header */
> + pkt->hdr.len = cpu_to_le32(payload_len);
>
> nbytes = copy_to_iter(&pkt->hdr, sizeof(pkt->hdr), &iov_iter);
> if (nbytes != sizeof(pkt->hdr)) {
> @@ -157,33 +173,47 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
> break;
> }
>
> - nbytes = copy_to_iter(pkt->buf, pkt->len, &iov_iter);
> - if (nbytes != pkt->len) {
> + nbytes = copy_to_iter(pkt->buf + pkt->off, payload_len,
> + &iov_iter);
> + if (nbytes != payload_len) {
> virtio_transport_free_pkt(pkt);
> vq_err(vq, "Faulted on copying pkt buf\n");
> break;
> }
>
> - vhost_add_used(vq, head, sizeof(pkt->hdr) + pkt->len);
> + vhost_add_used(vq, head, sizeof(pkt->hdr) + payload_len);
> added = true;
>
> - if (pkt->reply) {
> - int val;
> -
> - val = atomic_dec_return(&vsock->queued_replies);
> -
> - /* Do we have resources to resume tx processing? */
> - if (val + 1 == tx_vq->num)
> - restart_tx = true;
> - }
> -
> /* Deliver to monitoring devices all correctly transmitted
> * packets.
> */
> virtio_transport_deliver_tap_pkt(pkt);
>
> - total_len += pkt->len;
> - virtio_transport_free_pkt(pkt);
> + pkt->off += payload_len;
> + total_len += payload_len;
> +
> + /* If we didn't send all the payload we can requeue the packet
> + * to send it with the next available buffer.
> + */
> + if (pkt->off < pkt->len) {
> + spin_lock_bh(&vsock->send_pkt_list_lock);
> + list_add(&pkt->list, &vsock->send_pkt_list);
> + spin_unlock_bh(&vsock->send_pkt_list_lock);
> + } else {
> + if (pkt->reply) {
> + int val;
> +
> + val = atomic_dec_return(&vsock->queued_replies);
> +
> + /* Do we have resources to resume tx
> + * processing?
> + */
> + if (val + 1 == tx_vq->num)
> + restart_tx = true;
> + }
> +
> + virtio_transport_free_pkt(pkt);
> + }
> } while(likely(!vhost_exceeds_weight(vq, ++pkts, total_len)));
> if (added)
> vhost_signal(&vsock->dev, vq);
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 34a2b42313b7..56fab3f03d0e 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -97,8 +97,17 @@ static struct sk_buff *virtio_transport_build_skb(void *opaque)
> struct virtio_vsock_pkt *pkt = opaque;
> struct af_vsockmon_hdr *hdr;
> struct sk_buff *skb;
> + size_t payload_len;
> + void *payload_buf;
>
> - skb = alloc_skb(sizeof(*hdr) + sizeof(pkt->hdr) + pkt->len,
> + /* A packet could be split to fit the RX buffer, so we can retrieve
> + * the payload length from the header and the buffer pointer taking
> + * care of the offset in the original packet.
> + */
> + payload_len = le32_to_cpu(pkt->hdr.len);
> + payload_buf = pkt->buf + pkt->off;
> +
> + skb = alloc_skb(sizeof(*hdr) + sizeof(pkt->hdr) + payload_len,
> GFP_ATOMIC);
> if (!skb)
> return NULL;
> @@ -138,8 +147,8 @@ static struct sk_buff *virtio_transport_build_skb(void *opaque)
>
> skb_put_data(skb, &pkt->hdr, sizeof(pkt->hdr));
>
> - if (pkt->len) {
> - skb_put_data(skb, pkt->buf, pkt->len);
> + if (payload_len) {
> + skb_put_data(skb, payload_buf, payload_len);
> }
>
> return skb;
> --
> 2.20.1
>
