Re: [PATCH] virtio-spec: add field for scsi command size

[Paolo Bonzini <pbonzini@redhat.com>]

Il 20/06/2013 04:40, Rusty Russell ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>> Il 19/06/2013 10:24, Michael S. Tsirkin ha scritto:
>>>>> 2) We introduce VIRTIO_NET_F_ANY_LAYOUT and VIRTIO_BLK_F_ANY_LAYOUT
>>>>>    specifically for net and block (note the new names).
>>
>> So why not a transport feature?  Is it just because the SCSI commands
>> for virtio-blk also require a config space field?  Sorry if I missed
>> this upthread.
> 
> Mainly because I'm not sure that *all* devices are now safe.  Are they?

virtio-scsi's implementation in QEMU is not safe (been delaying that for
too long, sorry), but the spec is safe.

Paolo

> I had a stress test half-written for this, pasted below.
> 
> Otherwise I'd be happy to do both: use feature 25 for
> VIRTIO_F_ANY_LAYOUT and another feature bit for the virtio-blk layout
> change.
> 
> Cheers,
> Rusty.
> 
> virtio: CONFIG_VIRTIO_DEVICE_TORTURE
> 
> Virtio devices are not supposed to depend on the framing of the scatter-gather
> lists, but various implementations did.  Safeguard this in future by adding
> an option to deliberately create perverse descriptors.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 8d5bddb..99c0187 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -5,6 +5,22 @@ config VIRTIO
>  	  bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_LGUEST,
>  	  CONFIG_RPMSG or CONFIG_S390_GUEST.
>  
> +config VIRTIO_TORTURE
> +	bool "Virtio torture debugging"
> +	depends on VIRTIO && DEBUG_KERNEL
> +	help
> +
> +	  This makes the virtio_ring implementation stress-test
> +	  devices.  In particularly, creatively change the format of
> +	  requests to make sure that devices are properly implemented,
> +	  as well as implement various checks to ensure drivers are
> +	  correct.  This will make your virtual machine slow *and*
> +	  unreliable!  Say N.
> +
> +	  Put virtio_ring.device_torture to your boot commandline to
> +	  torture devices (otherwise only simply sanity checks are
> +	  done).
> +
>  menu "Virtio drivers"
>  
>  config VIRTIO_PCI
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index e82821a..6e5271c 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -45,35 +45,6 @@
>  #define virtio_wmb(vq) wmb()
>  #endif
>  
> -#ifdef DEBUG
> -/* For development, we want to crash whenever the ring is screwed. */
> -#define BAD_RING(_vq, fmt, args...)				\
> -	do {							\
> -		dev_err(&(_vq)->vq.vdev->dev,			\
> -			"%s:"fmt, (_vq)->vq.name, ##args);	\
> -		BUG();						\
> -	} while (0)
> -/* Caller is supposed to guarantee no reentry. */
> -#define START_USE(_vq)						\
> -	do {							\
> -		if ((_vq)->in_use)				\
> -			panic("%s:in_use = %i\n",		\
> -			      (_vq)->vq.name, (_vq)->in_use);	\
> -		(_vq)->in_use = __LINE__;			\
> -	} while (0)
> -#define END_USE(_vq) \
> -	do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; } while(0)
> -#else
> -#define BAD_RING(_vq, fmt, args...)				\
> -	do {							\
> -		dev_err(&_vq->vq.vdev->dev,			\
> -			"%s:"fmt, (_vq)->vq.name, ##args);	\
> -		(_vq)->broken = true;				\
> -	} while (0)
> -#define START_USE(vq)
> -#define END_USE(vq)
> -#endif
> -
>  struct indirect_cache {
>  	unsigned int max;
>  	struct vring_desc *cache;
> @@ -109,7 +80,7 @@ struct vring_virtqueue
>  	/* How to notify other side. FIXME: commonalize hcalls! */
>  	void (*notify)(struct virtqueue *vq);
>  
> -#ifdef DEBUG
> +#ifdef CONFIG_VIRTIO_TORTURE
>  	/* They're supposed to lock for us. */
>  	unsigned int in_use;
>  
> @@ -134,6 +105,200 @@ static inline struct indirect_cache *indirect_cache(struct vring_virtqueue *vq)
>  	return (struct indirect_cache *)&vq->data[vq->vring.num];
>  }
>  
> +#ifdef CONFIG_VIRTIO_TORTURE
> +/* For development, we want to crash whenever the ring is screwed. */
> +#define BAD_RING(_vq, fmt, args...)				\
> +	do {							\
> +		dev_err(&(_vq)->vq.vdev->dev,			\
> +			"%s:"fmt, (_vq)->vq.name, ##args);	\
> +		BUG();						\
> +	} while (0)
> +/* Caller is supposed to guarantee no reentry. */
> +#define START_USE(_vq)						\
> +	do {							\
> +		if ((_vq)->in_use)				\
> +			panic("%s:in_use = %i\n",		\
> +			      (_vq)->vq.name, (_vq)->in_use);	\
> +		(_vq)->in_use = __LINE__;			\
> +	} while (0)
> +#define END_USE(_vq) \
> +	do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; } while(0)
> +
> +static bool device_torture;
> +module_param(device_torture, bool, 0644);
> +
> +struct torture {
> +	unsigned int orig_out, orig_in;
> +	void *orig_data;
> +	struct scatterlist sg[4];
> +	struct scatterlist orig_sg[];
> +};
> +
> +static unsigned tot_len(struct scatterlist sg[], unsigned num)
> +{
> +	unsigned len, i;
> +
> +	for (len = 0, i = 0; i < num; i++)
> +		len += sg[i].length;
> +
> +	return len;
> +}
> +
> +static void copy_sg_data(const struct scatterlist *dst, unsigned dnum,
> +			 const struct scatterlist *src, unsigned snum)
> +{
> +	unsigned len;
> +	struct scatterlist s, d;
> +
> +	s = *src;
> +	d = *dst;
> +
> +	while (snum && dnum) {
> +		len = min(s.length, d.length);
> +		memcpy(sg_virt(&d), sg_virt(&s), len);
> +		d.offset += len;
> +		d.length -= len;
> +		s.offset += len;
> +		s.length -= len;
> +		if (!s.length) {
> +			BUG_ON(snum == 0);
> +			src++;
> +			snum--;
> +			s = *src;
> +		}
> +		if (!d.length) {
> +			BUG_ON(dnum == 0);
> +			dst++;
> +			dnum--;
> +			d = *dst;
> +		}
> +	}
> +}
> +
> +static bool torture_replace(struct scatterlist **sg,
> +			     unsigned int *out,
> +			     unsigned int *in,
> +			     void **data,
> +			     gfp_t gfp)
> +{
> +	static size_t seed;
> +	struct torture *t;
> +	unsigned long outlen, inlen, ourseed, len1;
> +	void *buf;
> +
> +	if (!device_torture)
> +		return true;
> +
> +	outlen = tot_len(*sg, *out);
> +	inlen = tot_len(*sg + *out, *in);
> +
> +	/* This will break horribly on large block requests. */
> +	t = kmalloc(sizeof(*t) + (*out + *in) * sizeof(t->orig_sg[1])
> +		    + outlen + 1 + inlen + 1, gfp);
> +	if (!t)
> +		return false;
> +
> +	sg_init_table(t->sg, 4);
> +	buf = &t->orig_sg[*out + *in];
> +
> +	memcpy(t->orig_sg, *sg, sizeof(**sg) * (*out + *in));
> +	t->orig_out = *out;
> +	t->orig_in = *in;
> +	t->orig_data = *data;
> +	*data = t;
> +
> +	ourseed = ACCESS_ONCE(seed);
> +	seed++;
> +
> +	*sg = t->sg;
> +	if (outlen) {
> +		/* Split outbuf into two parts, one byte apart. */
> +		*out = 2;
> +		len1 = ourseed % (outlen + 1);
> +		sg_set_buf(&t->sg[0], buf, len1);
> +		buf += len1 + 1;
> +		sg_set_buf(&t->sg[1], buf, outlen - len1);
> +		buf += outlen - len1;
> +		copy_sg_data(t->sg, *out, t->orig_sg, t->orig_out);
> +	}
> +
> +	if (inlen) {
> +		/* Split inbuf into two parts, one byte apart. */
> +		*in = 2;
> +		len1 = ourseed % (inlen + 1);
> +		sg_set_buf(&t->sg[*out], buf, len1);
> +		buf += len1 + 1;
> +		sg_set_buf(&t->sg[*out + 1], buf, inlen - len1);
> +		buf += inlen - len1;
> +	}
> +	return true;
> +}
> +
> +static void *torture_done(struct torture *t)
> +{
> +	void *data;
> +
> +	if (!device_torture)
> +		return t;
> +
> +	if (t->orig_in)
> +		copy_sg_data(t->orig_sg + t->orig_out, t->orig_in,
> +			     t->sg + (t->orig_out ? 2 : 0), 2);
> +
> +	data = t->orig_data;
> +	kfree(t);
> +	return data;
> +}
> +
> +static unsigned long tot_inlen(struct virtqueue *vq, unsigned int i)
> +{
> +	struct vring_desc *desc;
> +	unsigned long len = 0;
> +
> +	if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT) {
> +		unsigned int num = vq->vring.desc[i].len / sizeof(*desc);
> +		desc = phys_to_virt(vq->vring.desc[i].addr);
> +
> +		for (i = 0; i < num; i++) {
> +			if (desc[i].flags & VRING_DESC_F_WRITE)
> +				len += desc[i].flags.len;
> +		}
> +	} else {
> +		desc = vq->vring.desc;
> +		while (desc[i].flags & VRING_DESC_F_NEXT) {
> +			if (desc[i].flags & VRING_DESC_F_WRITE)
> +				len += desc[i].flags.len;
> +			i = desc[i].next;
> +		}
> +	}
> +
> +	return len;
> +}
> +#else
> +static bool torture_replace(struct scatterlist **sg,
> +			     unsigned int *out,
> +			     unsigned int *in,
> +			     void **data,
> +			     gfp_t gfp)
> +{
> +	return true;
> +}
> +
> +static void *torture_done(void *data)
> +{
> +	return data;
> +}
> +
> +#define BAD_RING(_vq, fmt, args...)				\
> +	do {							\
> +		dev_err(&_vq->vq.vdev->dev,			\
> +			"%s:"fmt, (_vq)->vq.name, ##args);	\
> +		(_vq)->broken = true;				\
> +	} while (0)
> +#define START_USE(vq)
> +#define END_USE(vq)
> +#endif /* CONFIG_VIRTIO_TORTURE */
> +
>  /* Set up an indirect table of descriptors and add it to the queue. */
>  static int vring_add_indirect(struct vring_virtqueue *vq,
>  			      struct scatterlist sg[],
> @@ -228,7 +393,10 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>  
>  	BUG_ON(data == NULL);
>  
> -#ifdef DEBUG
> +	if (!torture_replace(&sg, &out, &in, &data, gfp))
> +		return -ENOMEM;
> +
> +#ifdef CONFIG_VIRTIO_TORTURE
>  	{
>  		ktime_t now = ktime_get();
>  
> @@ -261,6 +429,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>  		if (out)
>  			vq->notify(&vq->vq);
>  		END_USE(vq);
> +		torture_done(data);
>  		return -ENOSPC;
>  	}
>  
> @@ -341,7 +510,7 @@ bool virtqueue_kick_prepare(struct virtqueue *_vq)
>  	new = vq->vring.avail->idx;
>  	vq->num_added = 0;
>  
> -#ifdef DEBUG
> +#ifdef CONFIG_VIRTIO_TORTURE
>  	if (vq->last_add_time_valid) {
>  		WARN_ON(ktime_to_ms(ktime_sub(ktime_get(),
>  					      vq->last_add_time)) > 100);
> @@ -474,6 +643,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
>  		return NULL;
>  	}
>  
> +#ifdef CONFIG_VIRTIO_TORTURE
> +	if (unlikely(tot_inlen(vq, i) < *len)) {
> +		BAD_RING(vq, "id %u: %u > %u used!\n",
> +			 i, *len, tot_inlen(vq, i));
> +		return NULL;
> +	}
> +#endif
> +
>  	/* detach_buf clears data, so grab it now. */
>  	ret = vq->data[i];
>  	detach_buf(vq, i);
> @@ -486,12 +663,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
>  		virtio_mb(vq);
>  	}
>  
> -#ifdef DEBUG
> +#ifdef CONFIG_VIRTIO_TORTURE
>  	vq->last_add_time_valid = false;
> +	BUG_ON(*len > 
> +
>  #endif
>  
>  	END_USE(vq);
> -	return ret;
> +	return torture_done(ret);
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_get_buf);
>  
> @@ -683,7 +862,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
>  	vq->last_used_idx = 0;
>  	vq->num_added = 0;
>  	list_add_tail(&vq->vq.list, &vdev->vqs);
> -#ifdef DEBUG
> +#ifdef CONFIG_VIRTIO_TORTURE
>  	vq->in_use = false;
>  	vq->last_add_time_valid = false;
>  #endif
>