[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset

From: Jason Wang
Subject: Re: [Qemu-devel] [PATCH] virtio: destroy region cache during reset
Date: Wed, 8 Mar 2017 17:51:22 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0

On 2017年03月08日 17:19, Cornelia Huck wrote:
On Wed, 8 Mar 2017 11:18:27 +0800
Jason Wang <address@hidden> wrote:

On 2017年03月07日 18:16, Cornelia Huck wrote:
On Tue,  7 Mar 2017 16:47:58 +0800
Jason Wang <address@hidden> wrote:

We don't destroy region cache during reset which can make the maps
of previous driver leaked to a buggy or malicious driver that don't
set vring address before starting to use the device. Fix this by
destroy the region cache during reset and validate it before trying to
use them. While at it, also validate address_space_cache_init() during
virtio_init_region_cache() to make sure we have a correct region

Signed-off-by: Jason Wang <address@hidden>
   hw/virtio/virtio.c | 88 
   1 file changed, 76 insertions(+), 12 deletions(-)
@@ -190,6 +211,10 @@ static inline uint16_t vring_avail_flags(VirtQueue *vq)
       VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
       hwaddr pa = offsetof(VRingAvail, flags);
+    if (!caches) {
+        virtio_error(vq->vdev, "Cannot map avail flags");
I'm not sure that virtio_error is the right thing here; ending up in
this function with !caches indicates an error in our logic.
Probably not, this can be triggered by buggy guest.
I would think that even a buggy guest should not be able to trigger
accesses to vring structures that have not yet been set up. What am I

I think this may happen if a driver start the device without setting the vring address. In this case, region caches cache the mapping of previous driver. But maybe I was wrong.

An assert
might be better (and I hope we can sort out all of those errors exposed
by the introduction of region caches for 2.9...)
I thought we should avoid assert as much as possible in this case. But
if you and maintainer want an assert, it's also fine.
My personal rule-of-thumb:
- If it is something that can be triggered by the guest, or it is
something that is easily recovered, set the device to broken.
- If it is something that indicates that we messed up our internal
logic, use an assert.

I think arriving here with !caches indicates the second case; but if
there is a way for a guest to trigger this, setting the device to
broken would certainly be better.

Yes, broken seems better.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]