Re: [PATCH] virtio: fix reachable assertion due to stale value of cached region size

[Carlos López]

Hi,

On 1/3/23 23:03, Michael S. Tsirkin wrote:>> diff --git 
a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index f35178f5fc..db70c4976e 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1071,6 +1071,7 @@ static void virtqueue_split_get_avail_bytes(VirtQueue *vq,
> >       VirtIODevice *vdev = vq->vdev;
> >       unsigned int max, idx;
> >       unsigned int total_bufs, in_total, out_total;
> > +    MemoryRegionCache *desc_cache;
>
> why are you moving desc_cache here?

Just to make virtqueue_split_get_avail_bytes() and 
virtqueue_packed_get_avail_bytes() symmetrical, but I'll keep it out of v2.

> >       MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
> >       int64_t len = 0;
> >       int rc;
> > @@ -1078,15 +1079,13 @@ static void virtqueue_split_get_avail_bytes(VirtQueue 
> > *vq,
> >       idx = vq->last_avail_idx;
> >       total_bufs = in_total = out_total = 0;
> >   
> > -    max = vq->vring.num;
> > -
> >       while ((rc = virtqueue_num_heads(vq, idx)) > 0) {
> > -        MemoryRegionCache *desc_cache = &caches->desc;
> > -        unsigned int num_bufs;
> > +        unsigned int num_bufs = total_bufs;
> >           VRingDesc desc;
> >           unsigned int i;
> >   
> > -        num_bufs = total_bufs;
>
> nice cleanup but not a bugfix. Keep cleanups separate from fixes pls.

Ok makes sense.

> > +        desc_cache = &caches->desc;
>
> init as part of declaration seems cleaner.
>
> > +        max = vq->vring.num;
> >   
>
> can we move declaration of max here within the loop?
> will make sure the problem does not recur.

Yes, sounds good.

> >           if (!virtqueue_get_head(vq, idx++, &i)) {
> >               goto err;
> > @@ -1218,14 +1217,14 @@ static void virtqueue_packed_get_avail_bytes(VirtQueue 
> > *vq,
> >       wrap_counter = vq->last_avail_wrap_counter;
> >       total_bufs = in_total = out_total = 0;
> >   
> > -    max = vq->vring.num;
> > -
> >       for (;;) {
> >           unsigned int num_bufs = total_bufs;
> >           unsigned int i = idx;
> >           int rc;
> >   
> >           desc_cache = &caches->desc;
> > +        max = vq->vring.num;
> > +
>
>
> same question can we move declaration into the loop?

Same as above.

--
Carlos López
Security Engineer
SUSE Software Solutions