block/nvme: Remove memory leak

[PATCH] block/nvme: Remove memory leak

Posted by Philippe Mathieu-Daudé 4 years ago

Fixes: bdd6a90a9 ("Add VFIO based NVMe driver")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nvme.c b/block/nvme.c
index 7b7c0cc5d6..9f3c7ab819 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -163,6 +163,7 @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
     }
     r = qemu_vfio_dma_map(s->vfio, q->queue, bytes, false, &q->iova);
     if (r) {
+        qemu_vfree(q->queue);
         error_setg(errp, "Cannot map queue");
     }
 }
-- 
2.21.1

Re: [PATCH] block/nvme: Remove memory leak

Posted by Stefano Garzarella 4 years ago

On Tue, Apr 14, 2020 at 05:17:26PM +0200, Philippe Mathieu-Daudé wrote:
> Fixes: bdd6a90a9 ("Add VFIO based NVMe driver")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 7b7c0cc5d6..9f3c7ab819 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -163,6 +163,7 @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
>      }
>      r = qemu_vfio_dma_map(s->vfio, q->queue, bytes, false, &q->iova);
>      if (r) {
> +        qemu_vfree(q->queue);
>          error_setg(errp, "Cannot map queue");
>      }
>  }
> -- 
> 2.21.1
> 
> 

Are we adding a double free with this change?

IIUC when nvme_init_queue() fails in the nvme_create_queue_pair() we
already call the nvme_free_queue_pair() that frees sq.queue and
cq.queue.

I think your patch make the code cleaner, freeing the buffer near the
allocation during an error, maybe we can also set q->queue to NULL
after the qemu_vfree().

Thanks,
Stefano

Re: [PATCH] block/nvme: Remove memory leak

Posted by Philippe Mathieu-Daudé 4 years ago

On 4/15/20 9:46 AM, Stefano Garzarella wrote:
> On Tue, Apr 14, 2020 at 05:17:26PM +0200, Philippe Mathieu-Daudé wrote:
>> Fixes: bdd6a90a9 ("Add VFIO based NVMe driver")
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   block/nvme.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 7b7c0cc5d6..9f3c7ab819 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -163,6 +163,7 @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
>>       }
>>       r = qemu_vfio_dma_map(s->vfio, q->queue, bytes, false, &q->iova);
>>       if (r) {
>> +        qemu_vfree(q->queue);
>>           error_setg(errp, "Cannot map queue");
>>       }
>>   }
>> -- 
>> 2.21.1
>>
>>
> 
> Are we adding a double free with this change?
> 
> IIUC when nvme_init_queue() fails in the nvme_create_queue_pair() we
> already call the nvme_free_queue_pair() that frees sq.queue and
> cq.queue.

Argh you are right, I missed that...

> 
> I think your patch make the code cleaner, freeing the buffer near the
> allocation during an error, maybe we can also set q->queue to NULL
> after the qemu_vfree().
> 
> Thanks,
> Stefano
>