[Qemu-devel] [PATCH] nvme: Reset s->nr_queues upon open failure

[Qemu-devel] [PATCH] nvme: Reset s->nr_queues upon open failure

[Fam Zheng]

It is wrong to leave this field as 1, as nvme_close() called in the
error handling code in nvme_file_open() will use it and try to free
s->queues again.

Clear the fields to avoid double-free.

Cc: qemu-stable@nongnu.org
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/nvme.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/nvme.c b/block/nvme.c
index 6f71122bf5..7bdeb0ffce 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -666,6 +666,8 @@ fail_queue:
     nvme_free_queue_pair(bs, s->queues[0]);
 fail:
     g_free(s->queues);
+    s->queues = NULL;
+    s->nr_queues = 0;
     if (s->regs) {
         qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
     }
-- 
2.17.0

Re: [Qemu-devel] [PATCH] nvme: Reset s->nr_queues upon open failure

[Kevin Wolf]

Am 13.06.2018 um 09:45 hat Fam Zheng geschrieben:
> It is wrong to leave this field as 1, as nvme_close() called in the
> error handling code in nvme_file_open() will use it and try to free
> s->queues again.
> 
> Clear the fields to avoid double-free.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/nvme.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 6f71122bf5..7bdeb0ffce 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -666,6 +666,8 @@ fail_queue:
>      nvme_free_queue_pair(bs, s->queues[0]);
>  fail:
>      g_free(s->queues);
> +    s->queues = NULL;
> +    s->nr_queues = 0;
>      if (s->regs) {
>          qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
>      }

Hm... Basically all the cleanup is duplicated. It's not only
nvme_free_queue_pair(), but also qemu_vfio_pci_unmap_bar() and
qemu_vfio_close(). Are we sure it's intended to call them twice?

Maybe nvme_init() shouldn't clean up any of this and rely on the
later nvme_close() call to do that?

I also notice that the error handling code in nvme_init() has a
g_free(s->queues) and event_notifier_cleanup(&s->irq_notifier), which
nvme_close() doesn't. Are these leaks in nvme_close()?

Kevin