From 612bdaa44a8bed09cf8da15df0ad98c018fb5ae4 Mon Sep 17 00:00:00 2001
From: Lei Yin <[email protected]>
Date: Thu, 6 Apr 2023 23:39:11 +0800
Subject: [PATCH] nvme: fix double blk_mq_complete_request for timeout request
with low probability
When nvme_cancel_tagset traverses all tagsets and executes
nvme_cancel_request, this request may be executing blk_mq_free_request
that is called by nvme_rdma_complete_timed_out/nvme_tcp_complete_timed_out.
When blk_mq_free_request executes to WRITE_ONCE(rq->state, MQ_RQ_IDLE) and
__blk_mq_free_request(rq), it will cause double blk_mq_complete_request for
this request, and it will cause a null pointer error in the second
execution of this function because rq->mq_hctx has set to NULL in first
execution.
Signed-off-by: Lei Yin <[email protected]>
---
drivers/nvme/host/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 53ef028596c6..c1cc384f4f3e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -450,8 +450,8 @@ bool nvme_cancel_request(struct request *req, void *data)
dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
"Cancelling I/O %d", req->tag);
- /* don't abort one completed request */
- if (blk_mq_request_completed(req))
+ /* don't abort one completed or idle request */
+ if (blk_mq_rq_state(req) != MQ_RQ_IN_FLIGHT)
return true;
nvme_req(req)->status = NVME_SC_HOST_ABORTED_CMD;
--
2.39.1
On Thu, Apr 06, 2023 at 04:18:18PM +0000, Lei Lei2 Yin wrote:
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 53ef028596c6..c1cc384f4f3e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -450,8 +450,8 @@ bool nvme_cancel_request(struct request *req, void *data)
> dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
> "Cancelling I/O %d", req->tag);
>
> - /* don't abort one completed request */
> - if (blk_mq_request_completed(req))
> + /* don't abort one completed or idle request */
> + if (blk_mq_rq_state(req) != MQ_RQ_IN_FLIGHT)
> return true;
I was suspicious about this path too, and had the same change long ago, but
shelved it when I couldn't produce any errors there. But the change makes sense
to me!
Reviewed-by: Keith Busch <[email protected]>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 53ef028596c6..c1cc384f4f3e 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -450,8 +450,8 @@ bool nvme_cancel_request(struct request *req, void *data)
>> dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
>> "Cancelling I/O %d", req->tag);
>>
>> - /* don't abort one completed request */
>> - if (blk_mq_request_completed(req))
>> + /* don't abort one completed or idle request */
>> + if (blk_mq_rq_state(req) != MQ_RQ_IN_FLIGHT)
>> return true;
>
> I was suspicious about this path too, and had the same change long ago, but
> shelved it when I couldn't produce any errors there. But the change makes sense
> to me!
>
> Reviewed-by: Keith Busch <[email protected]>
We need to change nvmf_complete_timed_out_request() too.
Reviewed-by: Sagi Grimberg <[email protected]>
Thanks,
applied to nvme-6.4.