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

["Michael S. Tsirkin" <mst@redhat.com>]

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