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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Good for post-apocalyptic scenarios, like S/390 hotplug.
Signed-off-by: Rusty Russell <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
---
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
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 <[email protected]>
> ---
> 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)
>
Heinz Graalfs <[email protected]> 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 <[email protected]>
>> ---
>> 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.