LinuxLists.cc - [PATCH] nvme-rdma: Avoid double freeing of async event data

2020-03-09 21:08:44

Subject: [PATCH] nvme-rdma: Avoid double freeing of async event data

The timeout of identify cmd, which is invoked as part of admin queue
creation, can result in freeing of async event data both in
nvme_rdma_timeout handler and error handling path of
nvme_rdma_configure_admin queue thus causing NULL pointer reference.
Call Trace:
? nvme_rdma_setup_ctrl+0x223/0x800 [nvme_rdma]
nvme_rdma_create_ctrl+0x2ba/0x3f7 [nvme_rdma]
nvmf_dev_write+0xa54/0xcc6 [nvme_fabrics]
__vfs_write+0x1b/0x40
vfs_write+0xb2/0x1b0
ksys_write+0x61/0xd0
__x64_sys_write+0x1a/0x20
do_syscall_64+0x60/0x1e0
entry_SYSCALL_64_after_hwframe+0x44/0xa9

Signed-off-by: Prabhath Sajeepa <[email protected]>
Reviewed-by: Roland Dreier <[email protected]>
---
drivers/nvme/host/rdma.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 3e85c5c..0fe08c4 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -850,9 +850,11 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl,
if (new)
blk_mq_free_tag_set(ctrl->ctrl.admin_tagset);
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;
+ if (ctrl->async_event_sqe.data) {
+ 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

2020-03-09 21:55:23

by Max Gurtovoy

[permalink] [raw]

Subject: Re: [PATCH] nvme-rdma: Avoid double freeing of async event data

On 3/9/2020 11:07 PM, Prabhath Sajeepa wrote:
> The timeout of identify cmd, which is invoked as part of admin queue
> creation, can result in freeing of async event data both in
> nvme_rdma_timeout handler and error handling path of
> nvme_rdma_configure_admin queue thus causing NULL pointer reference.
> Call Trace:
> ? nvme_rdma_setup_ctrl+0x223/0x800 [nvme_rdma]
> nvme_rdma_create_ctrl+0x2ba/0x3f7 [nvme_rdma]
> nvmf_dev_write+0xa54/0xcc6 [nvme_fabrics]
> __vfs_write+0x1b/0x40
> vfs_write+0xb2/0x1b0
> ksys_write+0x61/0xd0
> __x64_sys_write+0x1a/0x20
> do_syscall_64+0x60/0x1e0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Signed-off-by: Prabhath Sajeepa <[email protected]>
> Reviewed-by: Roland Dreier <[email protected]>
> ---
> drivers/nvme/host/rdma.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 3e85c5c..0fe08c4 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -850,9 +850,11 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl,
> if (new)
> blk_mq_free_tag_set(ctrl->ctrl.admin_tagset);
> 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;
> + if (ctrl->async_event_sqe.data) {
> + 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;

Looks good,

Reviewed-by: Max Gurtovoy <[email protected]>

We did the same fix in-house yesterday and planed to send it tomorrow :)

2020-03-10 17:54:10

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] nvme-rdma: Avoid double freeing of async event data

On Mon, Mar 09, 2020 at 03:07:53PM -0600, Prabhath Sajeepa wrote:
> The timeout of identify cmd, which is invoked as part of admin queue
> creation, can result in freeing of async event data both in
> nvme_rdma_timeout handler and error handling path of
> nvme_rdma_configure_admin queue thus causing NULL pointer reference.
> Call Trace:
> ? nvme_rdma_setup_ctrl+0x223/0x800 [nvme_rdma]
> nvme_rdma_create_ctrl+0x2ba/0x3f7 [nvme_rdma]
> nvmf_dev_write+0xa54/0xcc6 [nvme_fabrics]
> __vfs_write+0x1b/0x40
> vfs_write+0xb2/0x1b0
> ksys_write+0x61/0xd0
> __x64_sys_write+0x1a/0x20
> do_syscall_64+0x60/0x1e0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Signed-off-by: Prabhath Sajeepa <[email protected]>
> Reviewed-by: Roland Dreier <[email protected]>

This looks good as a hot fix:

Reviewed-by: Christoph Hellwig <[email protected]>

But I really think we need to do something about init vs timeout
in the state machine. Otherwise we're going to deal with racing
resource deallocation forever.

2020-03-10 21:01:59

by Keith Busch

[permalink] [raw]