Changeset
block/nvme.c | 2 ++
1 file changed, 2 insertions(+)
Git apply log
Switched to a new branch '20180613074552.15656-1-famz@redhat.com'
Applying: nvme: Reset s->nr_queues upon open failure
To https://github.com/patchew-project/qemu
 * [new tag]         patchew/20180613074552.15656-1-famz@redhat.com -> patchew/20180613074552.15656-1-famz@redhat.com
Test passed: checkpatch

loading

Test passed: docker-mingw@fedora

loading

Test passed: s390x

loading

Test passed: docker-quick@centos7

loading

[Qemu-devel] [PATCH] nvme: Reset s->nr_queues upon open failure
Posted by Fam Zheng, 1 week ago
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
Posted by Kevin Wolf, 1 week ago
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