Re: [RFC PATCH 4/4] vhost_net: byteswap virtio_net header

Michael S. Tsirkin
2014-11-03T16:25:44Z

On Wed, Oct 29, 2014 at 09:38:45AM +0100, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>

This patch casts userspace pointers to void *,
this is generally unsafe. In particular sparse will warn.

> ---
> drivers/vhost/net.c | 39 ++++++++++++++++++++++++++++++---------
> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 8dae2f724a35..f2d5f585dae9 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -18,6 +18,7 @@
> #include <linux/file.h>
> #include <linux/slab.h>
> #include <linux/vmalloc.h>
> +#include <linux/swab.h>
>
> #include <linux/net.h>
> #include <linux/if_packet.h>
> @@ -329,6 +330,14 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
> rcu_read_unlock_bh();
> }
>
> +static void virtio_net_hdr_swap(struct virtio_net_hdr *hdr)
> +{
> + hdr->hdr_len = swab16(hdr->hdr_len);
> + hdr->gso_size = swab16(hdr->gso_size);
> + hdr->csum_start = swab16(hdr->csum_start);
> + hdr->csum_offset = swab16(hdr->csum_offset);
> +}
> +
> /* Expects to be always run from workqueue - which acts as
> * read-size critical section for our kind of RCU. */
> static void handle_tx(struct vhost_net *net)
> @@ -346,7 +355,7 @@ static void handle_tx(struct vhost_net *net)
> .msg_flags = MSG_DONTWAIT,
> };
> size_t len, total_len = 0;
> - int err;
> + int err, has_vnet_hdr;
> size_t hdr_size;
> struct socket *sock;
> struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
> @@ -359,6 +368,7 @@ static void handle_tx(struct vhost_net *net)
>
> vhost_disable_notify(&net->dev, vq);
>
> + has_vnet_hdr = vhost_has_feature(vq, VHOST_NET_F_VIRTIO_NET_HDR);
> hdr_size = nvq->vhost_hlen;
> zcopy = nvq->ubufs;
>
> @@ -406,6 +416,8 @@ static void handle_tx(struct vhost_net *net)
> break;
> }
>
> + if (!has_vnet_hdr && vq->byteswap)
> + virtio_net_hdr_swap((void *) vq->iov[0].iov_base);
> zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
> && (nvq->upend_idx + 1) % UIO_MAXIOV !=
> nvq->done_idx
> @@ -570,7 +582,7 @@ static void handle_rx(struct vhost_net *net)
> .hdr.gso_type = VIRTIO_NET_HDR_GSO_NONE
> };
> size_t total_len = 0;
> - int err, mergeable;
> + int err, mergeable, has_vnet_hdr;
> s16 headcount;
> size_t vhost_hlen, sock_hlen;
> size_t vhost_len, sock_len;
> @@ -588,6 +600,7 @@ static void handle_rx(struct vhost_net *net)
> vq_log = unlikely(vhost_has_feature(vq, VHOST_F_LOG_ALL)) ?
> vq->log : NULL;
> mergeable = vhost_has_feature(vq, VIRTIO_NET_F_MRG_RXBUF);
> + has_vnet_hdr = vhost_has_feature(vq, VHOST_NET_F_VIRTIO_NET_HDR);
>
> while ((sock_len = peek_head_len(sock->sk))) {
> sock_len += sock_hlen;
> @@ -638,6 +651,9 @@ static void handle_rx(struct vhost_net *net)
> vhost_discard_vq_desc(vq, headcount);
> continue;
> }
> +
> + if (!has_vnet_hdr && vq->byteswap)
> + virtio_net_hdr_swap((void *) vq->iov[0].iov_base);
> if (unlikely(vhost_hlen) &&
> memcpy_toiovecend(nvq->hdr, (unsigned char *)&hdr, 0,
> vhost_hlen)) {
> @@ -646,13 +662,18 @@ static void handle_rx(struct vhost_net *net)
> break;
> }
> /* TODO: Should check and handle checksum. */
> - if (likely(mergeable) &&
> - memcpy_toiovecend(nvq->hdr, (unsigned char *)&headcount,
> - offsetof(typeof(hdr), num_buffers),
> - sizeof hdr.num_buffers)) {
> - vq_err(vq, "Failed num_buffers write");
> - vhost_discard_vq_desc(vq, headcount);
> - break;
> + if (likely(mergeable)) {
> + __u16 tmp = headcount;
> +
> + if (vq->byteswap)
> + tmp = swab16(headcount);
> + if (memcpy_toiovecend(nvq->hdr, (unsigned char *)&tmp,
> + offsetof(typeof(hdr), num_buffers),
> + sizeof(hdr.num_buffers))) {
> + vq_err(vq, "Failed num_buffers write");
> + vhost_discard_vq_desc(vq, headcount);
> + break;
> + }
> }
> vhost_add_used_and_signal_n(&net->dev, vq, vq->heads,
> headcount);
> --
> 1.7.10.4