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

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Wed, 8 Mar 2017 11:18:27 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2017年03月07日 18:16, Cornelia Huck wrote:
> > On Tue,  7 Mar 2017 16:47:58 +0800
> > Jason Wang <jasowang@redhat.com> 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
> >> cache.
> >>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >>   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
missing?

> 
> > 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.

> 
> >
> >> +        return 0;
> >> +    }
> >>       return virtio_lduw_phys_cached(vq->vdev, &caches->avail, pa);
> >>   }
> >>

> >> @@ -1117,6 +1174,15 @@ static enum virtio_device_endian virtio_current_cpu_endian(void)
> >>       }
> >>   }
> >>
> >> +static void virtio_virtqueue_reset_region_cache(struct VirtQueue *vq)
> >> +{
> >> +    VRingMemoryRegionCaches *caches;
> >> +
> >> +    caches = atomic_read(&vq->vring.caches);
> >> +    atomic_set(&vq->vring.caches, NULL);
> >> +    virtio_free_region_cache(caches);
> > Shouldn't this use rcu to free it? Unconditionally setting caches to
> > NULL feels wrong...
> 
> Right, will switch to use rcu.
> 
> >> +}
> >> +
> >>   void virtio_reset(void *opaque)
> >>   {
> >>       VirtIODevice *vdev = opaque;
> >> @@ -1157,6 +1223,7 @@ void virtio_reset(void *opaque)
> >>           vdev->vq[i].notification = true;
> >>           vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
> >>           vdev->vq[i].inuse = 0;
> >> +        virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
> > ...especially as you call it in a reset context here.
> >
> >>       }
> >>   }
> >>
> >> @@ -2451,13 +2518,10 @@ static void virtio_device_free_virtqueues(VirtIODevice *vdev)
> >>       }
> >>
> >>       for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> >> -        VRingMemoryRegionCaches *caches;
> >>           if (vdev->vq[i].vring.num == 0) {
> >>               break;
> >>           }
> >> -        caches = atomic_read(&vdev->vq[i].vring.caches);
> >> -        atomic_set(&vdev->vq[i].vring.caches, NULL);
> >> -        virtio_free_region_cache(caches);
> >> +        virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
> > OTOH, immediate destruction may still be called for during device
> > finalization.
> >
> 
> Right but to avoid code duplication, use rcu unconditionally should be 
> no harm here.

Yes, this should be fine.