* [PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data
@ 2018-11-26 17:17 Prabhath Sajeepa
2018-11-26 17:26 ` Roland Dreier
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Prabhath Sajeepa @ 2018-11-26 17:17 UTC (permalink / raw)
Some error paths in configuration of admin queue free data buffer
associated with async request SQE without resetting the data buffer
pointer to NULL, This buffer is also freed up again if the controller
is shutdown or reset.
---
drivers/nvme/host/rdma.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index d181caf..ab6ec72 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -184,6 +184,7 @@ static int nvme_rdma_alloc_qe(struct ib_device *ibdev, struct nvme_rdma_qe *qe,
qe->dma = ib_dma_map_single(ibdev, qe->data, capsule_size, dir);
if (ib_dma_mapping_error(ibdev, qe->dma)) {
kfree(qe->data);
+ qe->data = NULL;
return -ENOMEM;
}
@@ -823,6 +824,7 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl,
out_free_async_qe:
nvme_rdma_free_qe(ctrl->device->dev, &ctrl->async_event_sqe,
sizeof(struct nvme_command), DMA_TO_DEVICE);
+ ctrl->async_event_sqe.data = NULL;
out_free_queue:
nvme_rdma_free_queue(&ctrl->queues[0]);
return error;
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data
2018-11-26 17:17 [PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data Prabhath Sajeepa
@ 2018-11-26 17:26 ` Roland Dreier
2018-11-27 7:33 ` Christoph Hellwig
2018-11-27 7:41 ` Sagi Grimberg
2 siblings, 0 replies; 12+ messages in thread
From: Roland Dreier @ 2018-11-26 17:26 UTC (permalink / raw)
On Mon, Nov 26, 2018 at 9:18 AM Prabhath Sajeepa
<psajeepa@purestorage.com> wrote:
> Some error paths in configuration of admin queue free data buffer
> associated with async request SQE without resetting the data buffer
> pointer to NULL, This buffer is also freed up again if the controller
> is shutdown or reset.
Reviewed-by: Roland Dreier <roland at purestorage.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data
2018-11-26 17:17 [PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data Prabhath Sajeepa
2018-11-26 17:26 ` Roland Dreier
@ 2018-11-27 7:33 ` Christoph Hellwig
2018-11-27 7:41 ` Sagi Grimberg
2 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2018-11-27 7:33 UTC (permalink / raw)
On Mon, Nov 26, 2018@10:17:39AM -0700, Prabhath Sajeepa wrote:
> Some error paths in configuration of admin queue free data buffer
> associated with async request SQE without resetting the data buffer
> pointer to NULL, This buffer is also freed up again if the controller
> is shutdown or reset.
Can you please provide a signoff?
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data
2018-11-26 17:17 [PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data Prabhath Sajeepa
2018-11-26 17:26 ` Roland Dreier
2018-11-27 7:33 ` Christoph Hellwig
@ 2018-11-27 7:41 ` Sagi Grimberg
2018-11-27 7:42 ` Christoph Hellwig
2 siblings, 1 reply; 12+ messages in thread
From: Sagi Grimberg @ 2018-11-27 7:41 UTC (permalink / raw)
Can you fix the change log title to "nvme-rdma: ..." please?
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data
@ 2018-11-20 20:11 Prabhath Sajeepa
2018-11-21 0:10 ` Sagi Grimberg
2018-11-21 0:21 ` Roland Dreier
0 siblings, 2 replies; 12+ messages in thread
From: Prabhath Sajeepa @ 2018-11-20 20:11 UTC (permalink / raw)
Some error paths in configuration of admin queue free data buffer
associated with async request SQE without resetting the data buffer
pointer to NULL, This buffer is also freed up again if the controller
is shutdown or reset.
---
drivers/nvme/host/rdma.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index d181caf..b61ae21 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -823,6 +823,7 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl,
out_free_async_qe:
nvme_rdma_free_qe(ctrl->device->dev, &ctrl->async_event_sqe,
sizeof(struct nvme_command), DMA_TO_DEVICE);
+ ctrl->async_event_sqe.data = NULL;
out_free_queue:
nvme_rdma_free_queue(&ctrl->queues[0]);
return error;
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data
2018-11-20 20:11 Prabhath Sajeepa
@ 2018-11-21 0:10 ` Sagi Grimberg
2018-11-21 0:21 ` Roland Dreier
1 sibling, 0 replies; 12+ messages in thread
From: Sagi Grimberg @ 2018-11-21 0:10 UTC (permalink / raw)
Reviewed-by: Sagi Grimberg <sagi at grimberg.me>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data
2018-11-20 20:11 Prabhath Sajeepa
2018-11-21 0:10 ` Sagi Grimberg
@ 2018-11-21 0:21 ` Roland Dreier
2018-11-21 0:30 ` Sagi Grimberg
1 sibling, 1 reply; 12+ messages in thread
From: Roland Dreier @ 2018-11-21 0:21 UTC (permalink / raw)
On Tue, Nov 20, 2018 at 12:11 PM Prabhath Sajeepa
<psajeepa@purestorage.com> wrote:
> Some error paths in configuration of admin queue free data buffer
> associated with async request SQE without resetting the data buffer
> pointer to NULL, This buffer is also freed up again if the controller
> is shutdown or reset.
> @@ -823,6 +823,7 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl,
> out_free_async_qe:
> nvme_rdma_free_qe(ctrl->device->dev, &ctrl->async_event_sqe,
> sizeof(struct nvme_command), DMA_TO_DEVICE);
> + ctrl->async_event_sqe.data = NULL;
> out_free_queue:
> nvme_rdma_free_queue(&ctrl->queues[0]);
> return error;
Fix looks good to me, although while reviewing this I noticed we have
a related bug too.
In nvme_rdma_alloc_qe() we do:
qe->data = kzalloc(capsule_size, GFP_KERNEL);
if (!qe->data)
return -ENOMEM;
qe->dma = ib_dma_map_single(ibdev, qe->data, capsule_size, dir);
if (ib_dma_mapping_error(ibdev, qe->dma)) {
kfree(qe->data);
return -ENOMEM;
}
if the ib_dma_map_single() fails then we'll free qe->data but not NULL
it out. If this happens for async_event_sqe in
nvme_rdma_configure_admin_queue() then we'll be vulnerable to the same
double free in nvme_rdma_destroy_admin_queue() that this patch is
fixing.
Not sure if that's worth fixing in the same patch here; but I guess we
should NULL out qe->data if the DMA mapping fails in alloc_qe...
- R.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data
2018-11-21 0:21 ` Roland Dreier
@ 2018-11-21 0:30 ` Sagi Grimberg
[not found] ` <CAE=VkfB4G8PFZuNihQR0fei9BSr5ayqOrMWjFq9Xo5anQNgk7w@mail.gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Sagi Grimberg @ 2018-11-21 0:30 UTC (permalink / raw)
> Fix looks good to me, although while reviewing this I noticed we have
> a related bug too.
>
> In nvme_rdma_alloc_qe() we do:
>
> qe->data = kzalloc(capsule_size, GFP_KERNEL);
> if (!qe->data)
> return -ENOMEM;
>
> qe->dma = ib_dma_map_single(ibdev, qe->data, capsule_size, dir);
> if (ib_dma_mapping_error(ibdev, qe->dma)) {
> kfree(qe->data);
> return -ENOMEM;
> }
>
> if the ib_dma_map_single() fails then we'll free qe->data but not NULL
> it out. If this happens for async_event_sqe in
> nvme_rdma_configure_admin_queue() then we'll be vulnerable to the same
> double free in nvme_rdma_destroy_admin_queue() that this patch is
> fixing.
Fully agree, good catch!
> Not sure if that's worth fixing in the same patch here; but I guess we
> should NULL out qe->data if the DMA mapping fails in alloc_qe...
Probably makes sense to have it in the same patch.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-11-27 7:42 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-26 17:17 [PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data Prabhath Sajeepa
2018-11-26 17:26 ` Roland Dreier
2018-11-27 7:33 ` Christoph Hellwig
2018-11-27 7:41 ` Sagi Grimberg
2018-11-27 7:42 ` Christoph Hellwig
-- strict thread matches above, loose matches on Subject: below --
2018-11-20 20:11 Prabhath Sajeepa
2018-11-21 0:10 ` Sagi Grimberg
2018-11-21 0:21 ` Roland Dreier
2018-11-21 0:30 ` Sagi Grimberg
[not found] ` <CAE=VkfB4G8PFZuNihQR0fei9BSr5ayqOrMWjFq9Xo5anQNgk7w@mail.gmail.com>
2018-11-21 1:57 ` Roland Dreier
[not found] ` <CAE=VkfBvD9gG3MuwptCdpZso3t0JsFTvO-s1q+aW9=Zm4QTR=Q@mail.gmail.com>
2018-11-21 4:37 ` Roland Dreier
2018-11-22 2:57 ` Sagi Grimberg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.