Message ID | 20200414151727.17012-1-philmd@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | block/nvme: Remove memory leak | expand |
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
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 >
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"); } }
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(+)