Re: [PATCH 03/32] irq: add per-IRQ observer to fix qemu_irq_intercept_in leak

[Fabiano Rosas <farosas@suse.de>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> qemu_irq_intercept_in() saves original IRQ handlers by allocating
> new QOM objects, which are never freed. On a PC machine, this leaks
> IRQ objects (one per IOAPIC pin) on every qtest run.
>
> Rather than tracking allocations to free later, avoid them: add an
> "observer" field to IRQState, called by qemu_set_irq() after the
> real handler. Interception sets the observer instead of rewriting
> handlers, so there's nothing to save and nothing to leak.
>
> Fix qemu_notirq() to route through qemu_set_irq() so inverted IRQs
> trigger observers too. Drop the LSan suppression.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/hw/core/irq.h         |  1 +
>  hw/core/irq.c                 | 10 +++++-----
>  system/qtest.c                |  3 ---
>  scripts/lsan_suppressions.txt |  8 --------
>  4 files changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/include/hw/core/irq.h b/include/hw/core/irq.h
> index 291fdd67df4..93d5710a73e 100644
> --- a/include/hw/core/irq.h
> +++ b/include/hw/core/irq.h
> @@ -14,6 +14,7 @@ struct IRQState {
>      qemu_irq_handler handler;
>      void *opaque;
>      int n;
> +    qemu_irq_handler observer;
>  };
>  
>  void qemu_set_irq(qemu_irq irq, int level);
> diff --git a/hw/core/irq.c b/hw/core/irq.c
> index 106805e2417..fa11e9bc0aa 100644
> --- a/hw/core/irq.c
> +++ b/hw/core/irq.c
> @@ -32,6 +32,9 @@ void qemu_set_irq(qemu_irq irq, int level)
>          return;
>  
>      irq->handler(irq->opaque, irq->n, level);
> +    if (unlikely(irq->observer)) {
> +        irq->observer(irq->opaque, irq->n, level);
> +    }
>  }
>  
>  static void init_irq_fields(IRQState *irq, qemu_irq_handler handler,
> @@ -111,7 +114,7 @@ static void qemu_notirq(void *opaque, int line, int level)
>  {
>      IRQState *irq = opaque;
>  
> -    irq->handler(irq->opaque, irq->n, !level);
> +    qemu_set_irq(irq, !level);
>  }
>  
>  qemu_irq qemu_irq_invert(qemu_irq irq)
> @@ -124,11 +127,8 @@ qemu_irq qemu_irq_invert(qemu_irq irq)
>  void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n)
>  {
>      int i;
> -    qemu_irq *old_irqs = qemu_allocate_irqs(NULL, NULL, n);
>      for (i = 0; i < n; i++) {
> -        *old_irqs[i] = *gpio_in[i];
> -        gpio_in[i]->handler = handler;
> -        gpio_in[i]->opaque = &old_irqs[i];
> +        gpio_in[i]->observer = handler;
>      }
>  }
>  
> diff --git a/system/qtest.c b/system/qtest.c
> index a79d10d1361..b359a3e1c84 100644
> --- a/system/qtest.c
> +++ b/system/qtest.c
> @@ -324,9 +324,6 @@ void qtest_sendf(CharFrontend *chr, const char *fmt, ...)
>  
>  static void qtest_irq_handler(void *opaque, int n, int level)
>  {
> -    qemu_irq old_irq = *(qemu_irq *)opaque;
> -    qemu_set_irq(old_irq, level);
> -
>      if (irq_levels[n] != level) {
>          CharFrontend *chr = &qtest->qtest_chr;
>          irq_levels[n] = level;
> diff --git a/scripts/lsan_suppressions.txt b/scripts/lsan_suppressions.txt
> index f88bbab18b8..30256bc6d01 100644
> --- a/scripts/lsan_suppressions.txt
> +++ b/scripts/lsan_suppressions.txt
> @@ -16,11 +16,3 @@ leak:/lib64/libxkbcommon.so.0
>  # https://github.com/GNOME/glib/blob/main/tools/glib.supp
>  # This avoids false positive leak reports for the qga-ssh-test.
>  leak:g_set_user_dirs
> -
> -# qemu_irq_intercept_in is only used by the qtest harness, and
> -# its API inherently involves a leak.
> -# While we could keep track of the old IRQ data structure
> -# in order to free it, it doesn't seem very important to fix
> -# since it is only used by the qtest test harness.
> -# Just ignore the leak, at least for the moment.
> -leak:qemu_irq_intercept_in

Reviewed-by: Fabiano Rosas <farosas@suse.de>