[PATCH 1/6] virtio_net: don't crash if virtqueue is broken.

[PATCH 1/6] virtio_net: don't crash if virtqueue is broken.

[Rusty Russell]

A bad implementation of virtio might cause us to mark the virtqueue
broken: we'll dev_err() in that case, and the device is useless, but
let's not BUG_ON().

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/virtio_net.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 5d776447d9c3..0949688ae540 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -904,8 +904,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 	sgs[out_num + in_num++] = &stat;
 
 	BUG_ON(out_num + in_num > ARRAY_SIZE(sgs));
-	BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC)
-	       < 0);
+	virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC);
 
 	if (unlikely(!virtqueue_kick(vi->cvq)))
 		return status == VIRTIO_NET_OK;
-- 
1.8.3.2

[PATCH 2/6] virtio_blk: don't crash, report error if virtqueue is broken.

[Rusty Russell]

A bad implementation of virtio might cause us to mark the virtqueue
broken: we'll dev_err() in that case, and the device is useless, but
let's not BUG_ON().

ENOMEM or ENOSPC implies the ring is full, and we should try again
later (-ENOMEM is documented to happen, but doesn't, as we fall
through to ENOSPC).

EIO means it's broken.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/block/virtio_blk.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 6a680d4de7f1..704d6c814c17 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -158,6 +158,7 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
 	unsigned long flags;
 	unsigned int num;
 	const bool last = (req->cmd_flags & REQ_END) != 0;
+	int err;
 
 	BUG_ON(req->nr_phys_segments + 2 > vblk->sg_elems);
 
@@ -198,11 +199,16 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
 	}
 
 	spin_lock_irqsave(&vblk->vq_lock, flags);
-	if (__virtblk_add_req(vblk->vq, vbr, vbr->sg, num) < 0) {
+	err = __virtblk_add_req(vblk->vq, vbr, vbr->sg, num);
+	if (err) {
 		virtqueue_kick(vblk->vq);
 		spin_unlock_irqrestore(&vblk->vq_lock, flags);
 		blk_mq_stop_hw_queue(hctx);
-		return BLK_MQ_RQ_QUEUE_BUSY;
+		/* Out of mem doesn't actually happen, since we fall back
+		 * to direct descriptors */
+		if (err == -ENOMEM || err == -ENOSPC)
+			return BLK_MQ_RQ_QUEUE_BUSY;
+		return BLK_MQ_RQ_QUEUE_ERROR;
 	}
 
 	if (last)
-- 
1.8.3.2

[PATCH 3/6] virtio_balloon: don't crash if virtqueue is broken.

[Rusty Russell]

A bad implementation of virtio might cause us to mark the virtqueue
broken: we'll dev_err() in that case, and the device is useless, but
let's not BUG().

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/virtio/virtio_balloon.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 5c4a95b516cf..fc1fb27dca49 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -108,8 +108,7 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
 	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
 
 	/* We should always be able to add one buffer to an empty queue. */
-	if (virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL) < 0)
-		BUG();
+	virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
 	virtqueue_kick(vq);
 
 	/* When host has read buffer, this completes via balloon_ack */
@@ -258,8 +257,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
 	if (!virtqueue_get_buf(vq, &len))
 		return;
 	sg_init_one(&sg, vb->stats, sizeof(vb->stats));
-	if (virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL) < 0)
-		BUG();
+	virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
 	virtqueue_kick(vq);
 }
 
@@ -338,7 +336,7 @@ static int init_vqs(struct virtio_balloon *vb)
 
 		/*
 		 * Prime this virtqueue with one buffer so the hypervisor can
-		 * use it to signal us later.
+		 * use it to signal us later (it can't be broken yet!).
 		 */
 		sg_init_one(&sg, vb->stats, sizeof vb->stats);
 		if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
-- 
1.8.3.2

[PATCH 4/6] virtio-rng: don't crash if virtqueue is broken.

[Rusty Russell]

A bad implementation of virtio might cause us to mark the virtqueue
broken: we'll dev_err() in that case, and the device is useless, but
let's not BUG().

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/char/hw_random/virtio-rng.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index c12398d1517c..2ce0e225e58c 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -47,8 +47,7 @@ static void register_buffer(u8 *buf, size_t size)
 	sg_init_one(&sg, buf, size);
 
 	/* There should always be room for one buffer. */
-	if (virtqueue_add_inbuf(vq, &sg, 1, buf, GFP_KERNEL) < 0)
-		BUG();
+	virtqueue_add_inbuf(vq, &sg, 1, buf, GFP_KERNEL);
 
 	virtqueue_kick(vq);
 }
-- 
1.8.3.2

[PATCH 5/6] virtio: fail adding buffer on broken queues.

[Rusty Russell]

Heinz points out that adding buffers to a broken virtqueue (which
should "never happen") still works.  Failing allows drivers to detect
and complain about broken devices.

Now drivers are robust, we can add this extra check.

Reported-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/virtio/virtio_ring.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 28b5338fff71..b74033dca384 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -203,6 +203,11 @@ static inline int virtqueue_add(struct virtqueue *_vq,
 
 	BUG_ON(data == NULL);
 
+	if (unlikely(vq->broken)) {
+		END_USE(vq);
+		return -EIO;
+	}
+
 #ifdef DEBUG
 	{
 		ktime_t now = ktime_get();
@@ -309,7 +314,7 @@ add_head:
  * Caller must ensure we don't call this with other virtqueue operations
  * at the same time (except where noted).
  *
- * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
+ * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
  */
 int virtqueue_add_sgs(struct virtqueue *_vq,
 		      struct scatterlist *sgs[],
@@ -347,7 +352,7 @@ EXPORT_SYMBOL_GPL(virtqueue_add_sgs);
  * Caller must ensure we don't call this with other virtqueue operations
  * at the same time (except where noted).
  *
- * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
+ * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
  */
 int virtqueue_add_outbuf(struct virtqueue *vq,
 			 struct scatterlist sg[], unsigned int num,
@@ -369,7 +374,7 @@ EXPORT_SYMBOL_GPL(virtqueue_add_outbuf);
  * Caller must ensure we don't call this with other virtqueue operations
  * at the same time (except where noted).
  *
- * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
+ * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
  */
 int virtqueue_add_inbuf(struct virtqueue *vq,
 			struct scatterlist sg[], unsigned int num,
-- 
1.8.3.2

[PATCH 6/6] virtio: virtio_break_device() to mark all virtqueues broken.

[Rusty Russell]

Good for post-apocalyptic scenarios, like S/390 hotplug.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/virtio/virtio_ring.c | 15 +++++++++++++++
 include/linux/virtio.h       |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b74033dca384..a84350019f62 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -864,4 +864,19 @@ bool virtqueue_is_broken(struct virtqueue *_vq)
 }
 EXPORT_SYMBOL_GPL(virtqueue_is_broken);
 
+/*
+ * This should prevent the device from being used, allowing drivers to
+ * recover.  You may need to grab appropriate locks to flush.
+ */
+void virtio_break_device(struct virtio_device *dev)
+{
+	struct virtqueue *_vq;
+
+	list_for_each_entry(_vq, &dev->vqs, list) {
+		struct vring_virtqueue *vq = to_vvq(_vq);
+		vq->broken = true;
+	}
+}
+EXPORT_SYMBOL_GPL(virtio_break_device);
+
 MODULE_LICENSE("GPL");
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index e4abb84199be..b46671e28de2 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -106,6 +106,8 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
 int register_virtio_device(struct virtio_device *dev);
 void unregister_virtio_device(struct virtio_device *dev);
 
+void virtio_break_device(struct virtio_device *dev);
+
 /**
  * virtio_driver - operations for a virtio I/O driver
  * @driver: underlying device driver (populate name and owner).
-- 
1.8.3.2

Re: [PATCH 2/6] virtio_blk: don't crash, report error if virtqueue is broken.

[Heinz Graalfs]

On 15/01/14 03:36, Rusty Russell wrote:
> A bad implementation of virtio might cause us to mark the virtqueue
> broken: we'll dev_err() in that case, and the device is useless, but
> let's not BUG_ON().
>
> ENOMEM or ENOSPC implies the ring is full, and we should try again
> later (-ENOMEM is documented to happen, but doesn't, as we fall
> through to ENOSPC).
>
> EIO means it's broken.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>   drivers/block/virtio_blk.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 6a680d4de7f1..704d6c814c17 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -158,6 +158,7 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
>   	unsigned long flags;
>   	unsigned int num;
>   	const bool last = (req->cmd_flags & REQ_END) != 0;
> +	int err;
>
>   	BUG_ON(req->nr_phys_segments + 2 > vblk->sg_elems);
>
> @@ -198,11 +199,16 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
>   	}
>
>   	spin_lock_irqsave(&vblk->vq_lock, flags);
> -	if (__virtblk_add_req(vblk->vq, vbr, vbr->sg, num) < 0) {
> +	err = __virtblk_add_req(vblk->vq, vbr, vbr->sg, num);
> +	if (err) {
>   		virtqueue_kick(vblk->vq);

the kick might fail here after a request was successfully added.

>   		spin_unlock_irqrestore(&vblk->vq_lock, flags);
>   		blk_mq_stop_hw_queue(hctx);
> -		return BLK_MQ_RQ_QUEUE_BUSY;
> +		/* Out of mem doesn't actually happen, since we fall back
> +		 * to direct descriptors */
> +		if (err == -ENOMEM || err == -ENOSPC)
> +			return BLK_MQ_RQ_QUEUE_BUSY;
> +		return BLK_MQ_RQ_QUEUE_ERROR;
>   	}
>
>   	if (last)
>

Re: [PATCH 2/6] virtio_blk: don't crash, report error if virtqueue is broken.

[Rusty Russell]

Heinz Graalfs <graalfs@linux.vnet.ibm.com> writes:
> On 15/01/14 03:36, Rusty Russell wrote:
>> A bad implementation of virtio might cause us to mark the virtqueue
>> broken: we'll dev_err() in that case, and the device is useless, but
>> let's not BUG_ON().
>>
>> ENOMEM or ENOSPC implies the ring is full, and we should try again
>> later (-ENOMEM is documented to happen, but doesn't, as we fall
>> through to ENOSPC).
>>
>> EIO means it's broken.
>>
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>> ---
>>   drivers/block/virtio_blk.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 6a680d4de7f1..704d6c814c17 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -158,6 +158,7 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
>>   	unsigned long flags;
>>   	unsigned int num;
>>   	const bool last = (req->cmd_flags & REQ_END) != 0;
>> +	int err;
>>
>>   	BUG_ON(req->nr_phys_segments + 2 > vblk->sg_elems);
>>
>> @@ -198,11 +199,16 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
>>   	}
>>
>>   	spin_lock_irqsave(&vblk->vq_lock, flags);
>> -	if (__virtblk_add_req(vblk->vq, vbr, vbr->sg, num) < 0) {
>> +	err = __virtblk_add_req(vblk->vq, vbr, vbr->sg, num);
>> +	if (err) {
>>   		virtqueue_kick(vblk->vq);
>
> the kick might fail here after a request was successfully added.

It could, but it we're already in the error path, so we can't do
anything about it.

>>   		spin_unlock_irqrestore(&vblk->vq_lock, flags);
>>   		blk_mq_stop_hw_queue(hctx);
>> -		return BLK_MQ_RQ_QUEUE_BUSY;
>> +		/* Out of mem doesn't actually happen, since we fall back
>> +		 * to direct descriptors */
>> +		if (err == -ENOMEM || err == -ENOSPC)
>> +			return BLK_MQ_RQ_QUEUE_BUSY;
>> +		return BLK_MQ_RQ_QUEUE_ERROR;
>>   	}
>>
>>   	if (last)
>>

Cheers,
Rusty.