Re: [virtio-dev] [PATCH v4 1/6] virtio-vsock vhost device: fixed endiannes issue

[Greg Kurz <gkurz@linux.vnet.ibm.com>]

On Thu, 31 Mar 2016 13:41:33 +0200
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> Added missing endianness initializations in the vhost-vsock device.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> ---
>  drivers/vhost/vsock.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 2c5963c..841b0bf 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -481,6 +481,7 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
>  		vq = &vsock->vqs[i];
>  		mutex_lock(&vq->mutex);
>  		vq->acked_features = features;
> +		vhost_init_is_le(vq);
>  		mutex_unlock(&vq->mutex);
>  	}
>  	mutex_unlock(&vsock->dev.mutex);

Hi Claudio !

Unless I missed something, we still have:

drivers/vhost/vhost.c:static void vhost_init_is_le(struct vhost_virtqueue *vq)

and even if you make vhost_init_is_le() extern, this won't work if the device
is in legacy mode, and the host and guest have different endianness.

This is because vhost_init_is_le() computes vq->is_le as follows when we
have CONFIG_VHOST_CROSS_ENDIAN_LEGACY=y:

vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be;

and vq->user_be comes from the VHOST_SET_VRING_ENDIAN ioctl.

There's an ordering issue: if vhost_init_is_le() is called before userspace
has called VHOST_SET_VRING_ENDIAN, then vq->is_le will be wrong and the
device will be stale.

Looking at Stefan's tree, this is exactly what QEMU does:

vhost_vsock_set_status()
 vhost_dev_start()
  vhost_dev_set_features() --------------------> triggers call to vhost_init_is_le()
  ...
  vhost_virtqueue_start()
   vhost_virtqueue_set_vring_endian_legacy() --> sets vq->user_be

When cross-endian support was added, we decided to solve the ordering issue by
deferring the call to vhost_init_is_le() to device activation time (i.e. when
setting the endpoint for vhost-scsi or setting the backend for vhost-net).

vhost-sock is obviously different: it doesn't attach anything to vq->private_data
and doesn't call vhost_init_used()...

Not sure how to address this. I'll try to come back with a solution.

Cheers.

--
Greg