Re: [Qemu-devel] [PATCH V2 1/3] virtio: guard against NULL pfn

[Jason Wang]

On 2017年03月13日 17:55, Cornelia Huck wrote:
> On Mon, 13 Mar 2017 14:29:41 +0800
> Jason Wang <address@hidden> wrote:
>
> > To avoid access stale memory region cache after reset, this patch
> > check the existence of virtqueue pfn for all exported virtqueue access
> > helpers before trying to use them.
> >
> > Cc: Cornelia Huck <address@hidden>
> > Cc: Paolo Bonzini <address@hidden>
> > Signed-off-by: Jason Wang <address@hidden>
> > ---
> >   hw/virtio/virtio.c | 21 +++++++++++++++++++++
> >   1 file changed, 21 insertions(+)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index efce4b3..76cc81b 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -322,6 +322,10 @@ static int virtio_queue_empty_rcu(VirtQueue *vq)
> >           return 0;
> >       }
> >
> > +    if (unlikely(!vq->vring.avail)) {
> > +        return 0;
> Shouldn't that rather return !0 (denoting a non-existing queue as
> empty)?

Yes.

> > +    }
> > +
> >       return vring_avail_idx(vq) == vq->last_avail_idx;
> >   }
> >
> > @@ -333,6 +337,10 @@ int virtio_queue_empty(VirtQueue *vq)
> >           return 0;
> >       }
> >
> > +    if (unlikely(!vq->vring.avail)) {
> > +        return 0;
> Likewise.
>
> > +    }
> > +
> >       rcu_read_lock();
> >       empty = vring_avail_idx(vq) == vq->last_avail_idx;
> >       rcu_read_unlock();
> > @@ -431,6 +439,10 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement 
> > *elem,
> >           return;
> >       }
> >
> > +    if (unlikely(!vq->vring.used)) {
> > +        return;
> > +    }
> > +
> >       idx = (idx + vq->used_idx) % vq->vring.num;
> >
> >       uelem.id = elem->index;
> > @@ -448,6 +460,10 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
> >           return;
> >       }
> >
> > +    if (unlikely(!vq->vring.used)) {
> > +        return;
> > +    }
> > +
> >       /* Make sure buffer is written before we update index. */
> >       smp_wmb();
> >       trace_virtqueue_flush(vq, count);
> > @@ -546,6 +562,11 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int 
> > *in_bytes,
> >       int64_t len = 0;
> >       int rc;
> >
> > +    if (unlikely(!vq->vring.desc)) {
> > +        *in_bytes = *out_bytes = 0;
> I think you need to check for in_bytes and out_bytes being !NULL first.

Right, will fix this in v2.

Thanks

> > +        return;
> > +    }
> > +
> >       rcu_read_lock();
> >       idx = vq->last_avail_idx;
> >       total_bufs = in_total = out_total = 0;