[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[Prabhath Sajeepa]

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

[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[Roland Dreier]

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>

[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[Christoph Hellwig]

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?

[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[Sagi Grimberg]

Can you fix the change log title to "nvme-rdma: ..." please?

[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[Christoph Hellwig]

On Mon, Nov 26, 2018@11:41:09PM -0800, Sagi Grimberg wrote:
> Can you fix the change log title to "nvme-rdma: ..." please?

And that as well, yes.  Although I usually fix those bits up when
applying instead of maxing a fuzz :)

[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[Prabhath Sajeepa]

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

[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[Roland Dreier]

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.

[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[Sagi Grimberg]

> 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.

[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[Roland Dreier]

On Tue, Nov 20, 2018 at 5:45 PM Prabhath Sajeepa
<psajeepa@purestorage.com> wrote:

> So, Does it make sense to set
> qe->data = NULL immediately after kfree(qe->data) in nvme_rdma_free_qe() ?

I thought about suggesting that; I have no objection although most
calls of free_qe are in nvme_rdma_free_ring() where we free the
containing structure immediately after.

However that doesn't fix the bug I spotted where alloc_qe fails the
DMA mapping, because we don't call free_qe in that path.  I think we
should set qe->data to NULL in alloc_qe when the DMA mapping fails, or
else the caller needs to know that alloc_qe can return an error but
leave a non-NULL pointer in qe->data.

 - R.

[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[Roland Dreier]

On Tue, Nov 20, 2018 at 6:12 PM Prabhath Sajeepa
<psajeepa@purestorage.com> wrote:
> Yes, we need to set qe->data to NULL in alloc_qe failure path as well.
> I was suggesting setting of qe->data to NULL in nvme_rdma_free_qe() in addition to the fix suggested by you.

I'm ambivalent, I think either way of handling clearing data after
free_qe is fine.  Two out of the three calls to free_qe need the "set
to NULL", so I personally don't have a strong feeling about whether
open-coding the data=NULL or putting it in the free function is
better.

 - R.

[PATCH] drivers/nvme/host/rdma.c: Fix double freeing of async event data

[Sagi Grimberg]

> <psajeepa@purestorage.com> wrote:
>> Yes, we need to set qe->data to NULL in alloc_qe failure path as well.
>> I was suggesting setting of qe->data to NULL in nvme_rdma_free_qe() in addition to the fix suggested by you.
> 
> I'm ambivalent, I think either way of handling clearing data after
> free_qe is fine.  Two out of the three calls to free_qe need the "set
> to NULL", so I personally don't have a strong feeling about whether
> open-coding the data=NULL or putting it in the free function is
> better.

How about simply NULLing for the specific flow that Roland spotted in
addition to yours?