Re: [PATCH] virtio: validate split queue head count before popping

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Jul 10, 2026 at 09:34:54AM +0800, m'te'a physical wrote:
> I think virtio_queue_empty_rcu() is no longer needed after this change.
> 
> For this caller, the pieces it covered are either already handled elsewhere or
> kept explicitly in the split pop path.
> 
> The disabled-device check is already done by virtqueue_pop() before it calls
> virtqueue_split_pop().
> 
> The empty-queue check is covered by virtqueue_num_heads(): rc == 0 is the
> normal empty case, and rc < 0 is the invalid avail index distance case that
> this
> patch wants to reject.
> 
> The read barrier is also covered by virtqueue_num_heads(), since it does
> smp_rmb() when it returns a positive head count, before the caller reads the
> avail ring entry with virtqueue_get_head().
> 
> The only old check that I think should stay explicit in virtqueue_split_pop()
> is
> !vq->vring.avail. virtqueue_num_heads() does not check that, and the old
> behavior for an unconfigured split queue was to treat it as empty. So v2 keeps
> that guard and replaces the rest of virtio_queue_empty_rcu() with
> virtqueue_num_heads():

Yea this is our hacky way to check the mr cache is valid.


>     RCU_READ_LOCK_GUARD();
>     if (unlikely(!vq->vring.avail)) {
>         goto done;
>     }
> 
>     rc = virtqueue_num_heads(vq, vq->last_avail_idx);
>     if (rc <= 0) {
>         goto done;
>     }
> 
> With that, virtio_queue_empty_rcu() has no remaining users and can be removed.
> 
> If this looks right, I can send a v2 patch with this change.
> 
> Michael S. Tsirkin <mst@redhat.com> 于2026年7月9日周四 20:43写道：
> 
>     On Thu, Jul 09, 2026 at 06:05:29PM +0800, Jia Jia wrote:
>     > virtqueue_split_pop() reads the next split avail ring entry after
>     > virtio_queue_empty_rcu() reports work. Call virtqueue_num_heads() before
>     > consuming that entry, so an avail index distance larger than the queue
>     > size is rejected.
>     >
>     > virtqueue_num_heads() also keeps the read barrier needed before the avail
>     > ring entry is read.
>     >
>     > This prevents an invalid split queue state from being expanded into
>     > repeated device command processing.
>     >
>     > Link: https://gitlab.com/qemu-project/qemu/-/issues/3930
>     > Signed-off-by: Jia Jia <physicalmtea@gmail.com>
>     > ---
>     > Tested with qemu-system-x86_64 11.0.50, built from origin/master
>     > f893c46c3931 plus this patch, configured with:
>     >   --target-list=x86_64-softmmu --enable-kvm --disable-tcg
>     >
>     > The original virtio-iommu live-vring qtest reproducer used for the
>     > report completed successfully on the patched build without host OOM,
>     > confirming that this reproducer is fixed by the patch.
>     >
>     >  hw/virtio/virtio.c | 8 +++++---
>     >  1 file changed, 5 insertions(+), 3 deletions(-)
>     >
>     > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>     > index f4d86a3655..252bff8641 100644
>     > --- a/hw/virtio/virtio.c
>     > +++ b/hw/virtio/virtio.c
>     > @@ -1751,9 +1751,11 @@ static void *virtqueue_split_pop(VirtQueue *vq,
>     size_t sz)
>     >      if (virtio_queue_empty_rcu(vq)) {
>     >          goto done;
>     >      }
>     > -    /* Needed after virtio_queue_empty(), see comment in
>     > -     * virtqueue_num_heads(). */
>     > -    smp_rmb();
>     > +
>     > +    rc = virtqueue_num_heads(vq, vq->last_avail_idx);
>     > +    if (rc <= 0) {
>     > +        goto done;
>     > +    }
> 
> 
>     Do we still need virtio_queue_empty_rcu then? Why?
> 
>     >      /* When we start there are none of either input nor output. */
>     >      out_num = in_num = elem_entries = 0;
>     > --
>     > 2.34.1
> 
>