Skip to contentSkip to conversation

nvme-rdma: Fix possible double free in reconnect flow

PATCH Superseded Sagi Grimberg 3 participants 4 messages Oct 19, 2017 NVMe

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Sagi Grimberg

author maintainer
#0 lore raw

PATCH 1/1 nvme-rdma: Fix possible double free in reconnect flow

The fact that we free the async event buffer in nvme_rdma_destroy_admin_queue can cause us to free it more than once because this happens in every reconnect attempt (we rely on the queue state flags DELETING to avoid this for other resources).

A more complete fix is to not destroy the admin/io queues unconditionally on every reconnect attempt, but its a bit more extensive and will go in the next release.

Reported-by Yi Zhang <yi.zhang at redhat.com>
Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
Note that this might cause a minor conflict with nvme-4.15 patches, but it will be easy enough to resolve it.

 drivers/nvme/host/rdma.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/nvme/host/rdma.c +5 -2
index 92a03ff5fb4d..ec22da478ba3 100644
@@ -571,6 +571,11 @@ static void nvme_rdma_free_queue(struct nvme_rdma_queue *queue)
 	if (test_and_set_bit(NVME_RDMA_Q_DELETING, &queue->flags))
 		return;
 
+	if(nvme_rdma_queue_idx(queue) == 0)
+		nvme_rdma_free_qe(queue->device->dev,
+			&queue->ctrl->async_event_sqe,
+			sizeof(struct nvme_command), DMA_TO_DEVICE);
+
 	nvme_rdma_destroy_queue_ib(queue);
 	rdma_destroy_id(queue->cm_id);
 }
@@ -739,8 +744,6 @@ static struct blk_mq_tag_set *nvme_rdma_alloc_tagset(struct nvme_ctrl *nctrl,
 static void nvme_rdma_destroy_admin_queue(struct nvme_rdma_ctrl *ctrl,
 		bool remove)
 {
-	nvme_rdma_free_qe(ctrl->queues[0].device->dev, &ctrl->async_event_sqe,
-			sizeof(struct nvme_command), DMA_TO_DEVICE);
 	nvme_rdma_stop_queue(&ctrl->queues[0]);
 	if (remove) {
 		blk_cleanup_queue(ctrl->ctrl.admin_q);
Signature · 1 line
2.7.4
#1 lore raw

PATCH 1/1 nvme-rdma: Fix possible double free in reconnect flow

Quoted 5 lines · Sagi Grimberg

The fact that we free the async event buffer in nvme_rdma_destroy_admin_queue can cause us to free it more than once because this happens in every reconnect attempt (we rely on the queue state flags DELETING to avoid this for other resources).

Do you appen to know which commit introduced the regression? A fixes line/tag would be really nice here.

Thanks,
Johannes

Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
#2 lore raw

PATCH 1/1 nvme-rdma: Fix possible double free in reconnect flow

In reply to Sagi Grimberg
Quoted 5 lines · Sagi Grimberg

The fact that we free the async event buffer in nvme_rdma_destroy_admin_queue can cause us to free it more than once because this happens in every reconnect attempt (we rely on the queue state flags DELETING to avoid this for other resources).

Do you appen to know which commit introduced the regression? A fixes line/tag would be really nice here.

Thanks,
Johannes

Signature · 6 lines
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
#3 lore raw

PATCH 1/1 nvme-rdma: Fix possible double free in reconnect flow

In reply to Sagi Grimberg

drivers/nvme/host/rdma.c static void nvme_rdma_free_queue(struct nvme_rdma_queue *queue) · line 574

Quoted 1 line
drivers/nvme/host/rdma.c +1 -0
+	if(nvme_rdma_queue_idx(queue) == 0)

missing space before the opening brace, but I can fix that up.