Re: [PATCH] virtio-iommu: fix OOM due to unbounded call_rcu

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

On Thu, Jul 23, 2026 at 05:42:02PM +0200, Eric Auger wrote:
> Hi Michael,
> 
> On 7/23/26 1:58 PM, Michael S. Tsirkin wrote:
> > Currently, within virtio-iommu, handle_command processes the command vq
> > without any limits on the number of entries processed.
> > This can easily and repeatedly enable/disable multiple memory regions.
> > Within the memory code, this causes an accumulation of an
> > unbounded number of RCU-deferred FlatViews - each of these
> > is supposed to be freed with call_rcu, but that never happens
> > because the main thread never returns to the main loop.
> >
> > Given FlatView is big, it's easy to have this balloon out to multiple
> > Gigabytes of memory.
> >
> > Limit the loop defer any remaining work to a timer.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3930
> > Cc: Eric Auger <eric.auger@redhat.com>
> > Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/hw/virtio/virtio-iommu.h |  1 +
> >  hw/virtio/virtio-iommu.c         | 29 +++++++++++++++++++++++++++++
> >  3 files changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
> > index 3b86050f2c..1f265540ad 100644
> > --- a/include/hw/virtio/virtio-iommu.h
> > +++ b/include/hw/virtio/virtio-iommu.h
> > @@ -65,6 +65,7 @@ struct VirtIOIOMMU {
> >      GTree *domains;
> >      QemuRecMutex mutex;
> >      GTree *endpoints;
> > +    QEMUTimer *cmd_timer;
> >      bool boot_bypass;
> >      Notifier machine_done;
> >      bool granule_frozen;
> > diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> > index 08f7e8b783..533bd5073f 100644
> > --- a/hw/virtio/virtio-iommu.c
> > +++ b/hw/virtio/virtio-iommu.c
> > @@ -993,6 +993,18 @@ static int virtio_iommu_handle_probe(VirtIOIOMMU *s,
> >      return ret ? ret : virtio_iommu_probe(s, &req, buf);
> >  }
> >  
> > +static void virtio_iommu_handle_command(VirtIODevice *vdev, VirtQueue *vq);
> > +
> > +static void virtio_iommu_handle_command_timer(void *opaque)
> > +{
> > +    VirtIOIOMMU *s = opaque;
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > +
> > +    if (virtio_device_started(vdev, vdev->status) && !vdev->broken) {
> > +        virtio_iommu_handle_command(vdev, s->req_vq);
> So what does happen in terms of scheduling. Can't this run concurrently
> with the usual direct virtio_iommu_handle_command() path? 

BQL prevents that.


> > +    }
> > +}
> > +
> >  static void virtio_iommu_handle_command(VirtIODevice *vdev, VirtQueue *vq)
> >  {
> >      VirtIOIOMMU *s = VIRTIO_IOMMU(vdev);
> > @@ -1003,10 +1015,17 @@ static void virtio_iommu_handle_command(VirtIODevice *vdev, VirtQueue *vq)
> >      struct iovec *iov;
> >      void *buf = NULL;
> >      size_t sz;
> > +    unsigned int batch = 0;
> >  
> >      for (;;) {
> >          size_t output_size = sizeof(tail);
> >  
> > +        if (++batch > virtio_queue_get_num(vdev, virtio_get_queue_index(vq))) {
> how did you chose that condition? is it always safe wrt oom situation?

It's kinda random, but the idea is that queue size already gives us
a point on the memory/speed curve.

There's nothing that is "safe wrt oom" in all configs - qemu makes
no promises and even a single allocation can in theory push you
into oom if memory is tight.

But what this prevents is unbounded memory allocations which will trigger
oom at some point for sure.



> > +            timer_mod(s->cmd_timer,
> > +                      qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) + 1);
> > +            break;
> > +        }
> > +
> >          elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> >          if (!elem) {
> >              return;
> > @@ -1416,6 +1435,8 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
> >      s->req_vq = virtio_add_queue(vdev, VIOMMU_DEFAULT_QUEUE_SIZE,
> >                               virtio_iommu_handle_command);
> >      s->event_vq = virtio_add_queue(vdev, VIOMMU_DEFAULT_QUEUE_SIZE, NULL);
> > +    s->cmd_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
> > +                                virtio_iommu_handle_command_timer, s);
> >  
> >      /*
> >       * config.bypass is needed to get initial address space early, such as
> > @@ -1498,6 +1519,7 @@ static void virtio_iommu_device_unrealize(DeviceState *dev)
> >  
> >      qemu_rec_mutex_destroy(&s->mutex);
> >  
> > +    timer_free(s->cmd_timer);
> >      virtio_delete_queue(s->req_vq);
> >      virtio_delete_queue(s->event_vq);
> >      virtio_cleanup(vdev);
> > @@ -1509,6 +1531,8 @@ static void virtio_iommu_device_reset_exit(Object *obj, ResetType type)
> >  
> >      trace_virtio_iommu_device_reset_exit();
> >  
> > +    timer_del(s->cmd_timer);
> > +
> >      if (s->domains) {
> >          g_tree_destroy(s->domains);
> >      }
> > @@ -1628,6 +1652,11 @@ static int iommu_post_load(void *opaque, int version_id)
> >       * still correct.
> >       */
> >      virtio_iommu_switch_address_space_all(s);
> > +
> > +    if (virtio_device_started(VIRTIO_DEVICE(s), VIRTIO_DEVICE(s)->status)) {
> > +        timer_mod(s->cmd_timer,
> > +                  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) + 1);
> > +    }
> >      return 0;
> >  }
> >  
> Thanks
> 
> Eric