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

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

[Jia Jia]

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;
+    }
 
     /* When we start there are none of either input nor output. */
     out_num = in_num = elem_entries = 0;
-- 
2.34.1

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

[Michael S. Tsirkin]

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

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

[m'te'a physical]

[-- Attachment #1: Type: text/plain, Size: 3396 bytes --]

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():

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

[-- Attachment #2: Type: text/html, Size: 4236 bytes --]

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

[Michael S. Tsirkin]

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

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

[Jia Jia]

virtqueue_split_pop() reads the next split avail ring entry after
checking that the ring is configured. Call virtqueue_num_heads() before
consuming that entry, so an avail index distance larger than the queue
size is rejected.

virtqueue_num_heads() also reports an empty queue and keeps the read
barrier needed before the avail ring entry is read. Drop the now unused
virtio_queue_empty_rcu() helper.

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>
---
v2:
- Drop virtio_queue_empty_rcu() from the split pop path.
- Keep only the existing !vq->vring.avail early exit before
  virtqueue_num_heads().
- Remove the now unused virtio_queue_empty_rcu() helper.

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 | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f4d86a3655..68ec3f0751 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -716,26 +716,6 @@ static inline bool is_desc_avail(uint16_t flags, bool wrap_counter)
     return (avail != used) && (avail == wrap_counter);
 }
 
-/* Fetch avail_idx from VQ memory only when we really need to know if
- * guest has added some buffers.
- * Called within rcu_read_lock().  */
-static int virtio_queue_empty_rcu(VirtQueue *vq)
-{
-    if (virtio_device_disabled(vq->vdev)) {
-        return 1;
-    }
-
-    if (unlikely(!vq->vring.avail)) {
-        return 1;
-    }
-
-    if (vq->shadow_avail_idx != vq->last_avail_idx) {
-        return 0;
-    }
-
-    return vring_avail_idx(vq) == vq->last_avail_idx;
-}
-
 static int virtio_queue_split_empty(VirtQueue *vq)
 {
     bool empty;
@@ -1748,12 +1728,14 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
     address_space_cache_init_empty(&indirect_desc_cache);
 
     RCU_READ_LOCK_GUARD();
-    if (virtio_queue_empty_rcu(vq)) {
+    if (unlikely(!vq->vring.avail)) {
+        goto done;
+    }
+
+    rc = virtqueue_num_heads(vq, vq->last_avail_idx);
+    if (rc <= 0) {
         goto done;
     }
-    /* Needed after virtio_queue_empty(), see comment in
-     * virtqueue_num_heads(). */
-    smp_rmb();
 
     /* When we start there are none of either input nor output. */
     out_num = in_num = elem_entries = 0;
-- 
2.34.1

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

[Michael S. Tsirkin]

On Fri, Jul 10, 2026 at 03:35:17PM +0800, Jia Jia wrote:
> virtqueue_split_pop() reads the next split avail ring entry after
> checking that the ring is configured. Call virtqueue_num_heads() before
> consuming that entry, so an avail index distance larger than the queue
> size is rejected.
> 
> virtqueue_num_heads() also reports an empty queue and keeps the read
> barrier needed before the avail ring entry is read. Drop the now unused
> virtio_queue_empty_rcu() helper.
> 
> This prevents an invalid split queue state from being expanded into
> repeated device command processing.

We can summarize this as:

virtio: reduce code duplication for split ring

virtio_queue_empty_rcu duplicates virtqueue_num_heads
for no good reason, let's not do it. As a nice side effect,
we gain better handling for misbehaving guests.


> Link: https://gitlab.com/qemu-project/qemu/-/issues/3930


not sure this is very relevant.

> Signed-off-by: Jia Jia <physicalmtea@gmail.com>
> ---
> v2:
> - Drop virtio_queue_empty_rcu() from the split pop path.
> - Keep only the existing !vq->vring.avail early exit before
>   virtqueue_num_heads().
> - Remove the now unused virtio_queue_empty_rcu() helper.
> 
> 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 | 30 ++++++------------------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index f4d86a3655..68ec3f0751 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -716,26 +716,6 @@ static inline bool is_desc_avail(uint16_t flags, bool wrap_counter)
>      return (avail != used) && (avail == wrap_counter);
>  }
>  
> -/* Fetch avail_idx from VQ memory only when we really need to know if
> - * guest has added some buffers.
> - * Called within rcu_read_lock().  */
> -static int virtio_queue_empty_rcu(VirtQueue *vq)
> -{
> -    if (virtio_device_disabled(vq->vdev)) {
> -        return 1;
> -    }

This check is no longer there. intentional?

> -
> -    if (unlikely(!vq->vring.avail)) {
> -        return 1;
> -    }
> -
> -    if (vq->shadow_avail_idx != vq->last_avail_idx) {
> -        return 0;
> -    }
> -
> -    return vring_avail_idx(vq) == vq->last_avail_idx;
> -}
> -
>  static int virtio_queue_split_empty(VirtQueue *vq)
>  {
>      bool empty;
> @@ -1748,12 +1728,14 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
>      address_space_cache_init_empty(&indirect_desc_cache);
>  
>      RCU_READ_LOCK_GUARD();
> -    if (virtio_queue_empty_rcu(vq)) {
> +    if (unlikely(!vq->vring.avail)) {
> +        goto done;
> +    }
> +
> +    rc = virtqueue_num_heads(vq, vq->last_avail_idx);
> +    if (rc <= 0) {
>          goto done;
>      }
> -    /* Needed after virtio_queue_empty(), see comment in
> -     * virtqueue_num_heads(). */
> -    smp_rmb();
>  
>      /* When we start there are none of either input nor output. */
>      out_num = in_num = elem_entries = 0;
> -- 
> 2.34.1

[PATCH v3] virtio: reduce code duplication for split ring

[Jia Jia]

virtio_queue_empty_rcu duplicates virtqueue_num_heads
for no good reason, let's not do it. As a nice side effect,
we gain better handling for misbehaving guests.

The virtio_device_disabled() check in virtio_queue_empty_rcu
is redundant because virtqueue_split_pop() is only called through
virtqueue_pop(), which already performs the check.

Signed-off-by: Jia Jia <physicalmtea@gmail.com>
---
v3:
- Rework the commit message to describe split-ring code deduplication.
- Explain why the virtio_device_disabled() check is removed.
- Drop the issue link because this patch addresses the generic split-ring
  path rather than the virtio-iommu command-processing path.

 hw/virtio/virtio.c | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f4d86a3655..68ec3f0751 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -716,26 +716,6 @@ static inline bool is_desc_avail(uint16_t flags, bool wrap_counter)
     return (avail != used) && (avail == wrap_counter);
 }
 
-/* Fetch avail_idx from VQ memory only when we really need to know if
- * guest has added some buffers.
- * Called within rcu_read_lock().  */
-static int virtio_queue_empty_rcu(VirtQueue *vq)
-{
-    if (virtio_device_disabled(vq->vdev)) {
-        return 1;
-    }
-
-    if (unlikely(!vq->vring.avail)) {
-        return 1;
-    }
-
-    if (vq->shadow_avail_idx != vq->last_avail_idx) {
-        return 0;
-    }
-
-    return vring_avail_idx(vq) == vq->last_avail_idx;
-}
-
 static int virtio_queue_split_empty(VirtQueue *vq)
 {
     bool empty;
@@ -1748,12 +1728,14 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
     address_space_cache_init_empty(&indirect_desc_cache);
 
     RCU_READ_LOCK_GUARD();
-    if (virtio_queue_empty_rcu(vq)) {
+    if (unlikely(!vq->vring.avail)) {
+        goto done;
+    }
+
+    rc = virtqueue_num_heads(vq, vq->last_avail_idx);
+    if (rc <= 0) {
         goto done;
     }
-    /* Needed after virtio_queue_empty(), see comment in
-     * virtqueue_num_heads(). */
-    smp_rmb();
 
     /* When we start there are none of either input nor output. */
     out_num = in_num = elem_entries = 0;