Re: [Qemu-devel] [PATCH V3 2/3] virtio: destroy region cache during reset

[Michael S. Tsirkin]

On Wed, Mar 15, 2017 at 10:14:48AM +0800, Jason Wang wrote:
> 
> 
> On 2017年03月14日 17:29, Cornelia Huck wrote:
> > On Tue, 14 Mar 2017 11:01:41 +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
> > > see them.
> > > 
> > > Cc: Cornelia Huck <address@hidden>
> > > Cc: Paolo Bonzini <address@hidden>
> > > Signed-off-by: Jason Wang <address@hidden>
> > > ---
> > > Changes from V2:
> > > - introduce a helper and assert caches != NULL
> > > Changes from v1:
> > > - switch to use rcu in virtio_virtqueue_region_cache()
> > > - use unlikely() when needed
> > > ---
> > >   hw/virtio/virtio.c | 46 ++++++++++++++++++++++++++++++----------------
> > >   1 file changed, 30 insertions(+), 16 deletions(-)
> > > 
> > > @@ -249,11 +255,10 @@ static inline void vring_used_idx_set(VirtQueue 
> > > *vq, uint16_t val)
> > >   /* Called within rcu_read_lock().  */
> > >   static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
> > >   {
> > > -    VRingMemoryRegionCaches *caches = atomic_rcu_read(&vq->vring.caches);
> > > +    VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
> > >       VirtIODevice *vdev = vq->vdev;
> > >       hwaddr pa = offsetof(VRingUsed, flags);
> > >       uint16_t flags = virtio_lduw_phys_cached(vq->vdev, &caches->used, 
> > > pa);
> > > -
> > Unrelated whitespace change.
> 
> Right. If no more comments from any others. I think Michael can probably fix
> this during merge.
> 
> Thanks

I'd rather get patches that apply cleanly. Minor changes mean you
can keep reviewed-by tags.

> > 
> > >       virtio_stw_phys_cached(vdev, &caches->used, pa, flags | mask);
> > >       address_space_cache_invalidate(&caches->used, pa, sizeof(flags));
> > >   }
> > Other than that:
> > 
> > Reviewed-by: Cornelia Huck <address@hidden>
> > 
> >