From 5148d52554d6bcf6134786d40f1c6f9f22e18978 Mon Sep 17 00:00:00 2001
From: Lei Yin <[email protected]>
Date: Thu, 6 Apr 2023 23:39:11 +0800
Subject: [PATCH v2] 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.
With multipath, by injecting a large number of requests timed out, I have
reproduced the issue that caused kernel crashes in three versions of the
kernel(include 5.10.167, 6.2.10 and upstream version which compiled by
myself). The error stack is as follows:
[ 2777.253091] <TASK>
[ 2777.253102] nvme_failover_req+0x10a/0x120 [nvme_core]
[ 2777.255302] blk_complete_reqs+0x3e/0x60
[ 2777.255726] __do_softirq+0xb6/0x2ad
[ 2777.256139] ? __pfx_smpboot_thread_fn+0x10/0x10
[ 2777.256556] run_ksoftirqd+0x28/0x40
[ 2777.256978] smpboot_thread_fn+0xdb/0x1d0
[ 2777.257399] kthread+0xd7/0x100
[ 2777.257827] ? __pfx_kthread+0x10/0x10
[ 2777.258253] ret_from_fork+0x29/0x50
[ 2777.258695] </TASK>
Signed-off-by: Lei Yin <[email protected]>
---
drivers/nvme/host/core.c | 4 ++--
drivers/nvme/host/fabrics.h | 2 +-
2 files changed, 3 insertions(+), 3 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;
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index dcac3df8a5f7..a25a0118b722 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -197,7 +197,7 @@ static inline char *nvmf_ctrl_subsysnqn(struct nvme_ctrl *ctrl)
static inline void nvmf_complete_timed_out_request(struct request *rq)
{
- if (blk_mq_request_started(rq) && !blk_mq_request_completed(rq)) {
+ if (blk_mq_rq_state(rq) == MQ_RQ_IN_FLIGHT) {
nvme_req(rq)->status = NVME_SC_HOST_ABORTED_CMD;
blk_mq_complete_request(rq);
}
--
2.39.1
On 4/8/23 00:44, Lei Lei2 Yin wrote:
> From 5148d52554d6bcf6134786d40f1c6f9f22e18978 Mon Sep 17 00:00:00 2001
> From: Lei Yin <[email protected]>
> Date: Thu, 6 Apr 2023 23:39:11 +0800
> Subject: [PATCH v2] 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.
>
> With multipath, by injecting a large number of requests timed out, I have
> reproduced the issue that caused kernel crashes in three versions of the
> kernel(include 5.10.167, 6.2.10 and upstream version which compiled by
> myself). The error stack is as follows:
>
> [ 2777.253091] <TASK>
> [ 2777.253102] nvme_failover_req+0x10a/0x120 [nvme_core]
> [ 2777.255302] blk_complete_reqs+0x3e/0x60
> [ 2777.255726] __do_softirq+0xb6/0x2ad
> [ 2777.256139] ? __pfx_smpboot_thread_fn+0x10/0x10
> [ 2777.256556] run_ksoftirqd+0x28/0x40
> [ 2777.256978] smpboot_thread_fn+0xdb/0x1d0
> [ 2777.257399] kthread+0xd7/0x100
> [ 2777.257827] ? __pfx_kthread+0x10/0x10
> [ 2777.258253] ret_from_fork+0x29/0x50
> [ 2777.258695] </TASK>
>
> Signed-off-by: Lei Yin <[email protected]>
> ---
>
Can you please add blktests for this fix under nvme category ?
Looks good, it also addresses Sagi's comment about
nvmf_complete_timed_out_request().
Reviewed-by: Chaitanya Kulkarni <[email protected]>
-ck
On 4/8/23 01:04, Chaitanya Kulkarni wrote:
>
>> [ 2777.253091] <TASK>
>> [ 2777.253102] nvme_failover_req+0x10a/0x120 [nvme_core]
>> [ 2777.255302] blk_complete_reqs+0x3e/0x60
>> [ 2777.255726] __do_softirq+0xb6/0x2ad
>> [ 2777.256139] ? __pfx_smpboot_thread_fn+0x10/0x10
>> [ 2777.256556] run_ksoftirqd+0x28/0x40
>> [ 2777.256978] smpboot_thread_fn+0xdb/0x1d0
>> [ 2777.257399] kthread+0xd7/0x100
>> [ 2777.257827] ? __pfx_kthread+0x10/0x10
>> [ 2777.258253] ret_from_fork+0x29/0x50
>> [ 2777.258695] </TASK>
>>
>> Signed-off-by: Lei Yin <[email protected]>
>> ---
>>
> Can you please add blktests for this fix under nvme category ?
>
>
When you do please CC me and Shinichiro ([email protected]).
-ck