Re: [PATCH v7 10/17] nvme-tcp: Use CCR to recover controller that hits an error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] nvme-tcp: premature completion of in-flight requests in
  nvme_tcp_try_send()
- [High] nvme-tcp: non-multipath I/O retry exhaustion during
  NVME_CTRL_FENCING
- [High] nvme-tcp: race in nvme_tcp_fencing_work() skips err_work on
  reset
- [High] nvme-tcp: race between nvme_tcp_error_recovery() and
  nvme_tcp_stop_ctrl()
- [Medium] nvme-tcp: race in nvme_tcp_error_recovery() bypasses
  controller fencing
--

commit abb8e00ab3d00deda1ad0650e2f1521537edf566
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvme-tcp: Use CCR to recover controller that hits an error

This commit transitions a live nvme-tcp controller to NVME_CTRL_FENCING on
error to attempt cross-controller reset via fencing_work before proceeding
to NVME_CTRL_RESETTING, while holding timed-out requests until fencing
completes.

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 7ecc6c007f8d4..244c6c50cf990 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
[ ... ]
> @@ -651,6 +652,12 @@ static void nvme_tcp_init_recv_ctx(struct nvme_tcp_queue *queue)
>  
>  static void nvme_tcp_error_recovery(struct nvme_ctrl *ctrl)
>  {
> +	if (nvme_change_ctrl_state(ctrl, NVME_CTRL_FENCING)) {
> +		dev_warn(ctrl->device, "starting controller fencing\n");
> +		queue_work(nvme_wq, &to_tcp_ctrl(ctrl)->fencing_work);
> +		return;
> +	}

[Severity: High]
Does leaving the queues unquiesced while ctrl is in NVME_CTRL_FENCING
cause non-multipath requests to exhaust their retries and fail
immediately?

When a request is dispatched to nvme_tcp_queue_rq() during
NVME_CTRL_FENCING or NVME_CTRL_FENCED, nvme_check_ready() returns false
and nvme_tcp_queue_rq() calls nvme_fail_nonready_command() before
nvme_tcp_setup_cmd_pdu():

drivers/nvme/host/core.c:nvme_fail_nonready_command() {
	...
	if (state != NVME_CTRL_DELETING_NOIO &&
	    state != NVME_CTRL_DELETING &&
	    state != NVME_CTRL_FENCING &&
	    state != NVME_CTRL_FENCED &&
	    state != NVME_CTRL_DEAD &&
	    !test_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags) &&
	    !blk_noretry_request(rq) && !(rq->cmd_flags & REQ_NVME_MPATH))
		return BLK_STS_RESOURCE;

	if (!(rq->rq_flags & RQF_DONTPREP))
		nvme_clear_nvme_request(rq);

	return nvme_host_path_error(rq);
}

Because NVME_CTRL_FENCING and NVME_CTRL_FENCED are excluded from returning
BLK_STS_RESOURCE, a non-multipath request (without REQ_NVME_MPATH) sets
RQF_DONTPREP and completes with NVME_SC_HOST_PATH_ERROR.
nvme_decide_disposition() then chooses RETRY and nvme_retry_req() kicks
the requeue list with zero delay on the still-unquiesced queue.  The
request loops through nvme_tcp_queue_rq() five times in a tight loop until
retries reach nvme_max_retries, logs an uninitialized or stale command in
nvme_log_error(), and fails with BLK_STS_TRANSPORT instead of waiting for
controller recovery.

> +
>  	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
>  		return;

[Severity: Medium]
Can this fallback nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING) call
transition a newly NVME_CTRL_LIVE controller directly to
NVME_CTRL_RESETTING and bypass fencing?

In nvme-tcp, ctrl->state leaves NVME_CTRL_NEW before any queues or sockets
exist, so NVME_CTRL_RESETTING can only be entered from NVME_CTRL_LIVE or
NVME_CTRL_FENCED.  If a socket callback triggers nvme_tcp_error_recovery()
while nvme_tcp_setup_ctrl() is finishing connection setup, the two state
changes can interleave:

CPU0                                  CPU1
nvme_tcp_error_recovery()             nvme_tcp_setup_ctrl()
  nvme_change_ctrl_state(...,
    NVME_CTRL_FENCING) -> false
    (state is NVME_CTRL_CONNECTING)
                                        nvme_change_ctrl_state(...,
                                          NVME_CTRL_LIVE) -> true
                                          nvme_kick_requeue_lists()
  nvme_change_ctrl_state(...,
    NVME_CTRL_RESETTING) -> true
  queue_work(..., &err_work)          <-- skips fencing_work

Requests dispatched as the controller became NVME_CTRL_LIVE are then
canceled by err_work and retried on another path without fencing the
controller first.  Removing the NVME_CTRL_RESETTING fallback from
nvme_tcp_error_recovery() avoids this race, since nvme-tcp only needs to
trigger error recovery from NVME_CTRL_LIVE.

> @@ -2767,6 +2774,22 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
>  	nvme_tcp_reconnect_or_remove(ctrl, ret);
>  }
>  
> +static void nvme_tcp_fencing_work(struct work_struct *work)
> +{
> +	struct nvme_tcp_ctrl *tcp_ctrl = container_of(work,
> +			struct nvme_tcp_ctrl, fencing_work);
> +	struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
> +	unsigned long rem;
> +
> +	rem = nvme_fence_ctrl(ctrl);
> +	if (rem)
> +		dev_info(ctrl->device, "CCR failed, starting error recovery\n");
> +
> +	nvme_change_ctrl_state(ctrl, NVME_CTRL_FENCED);
> +	if (nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
> +		queue_work(nvme_reset_wq, &tcp_ctrl->err_work);
> +}

[Severity: High]
Can another caller claim the NVME_CTRL_FENCED to NVME_CTRL_RESETTING
transition between these two nvme_change_ctrl_state() calls and prevent
tcp_ctrl->err_work from running?

nvme_change_ctrl_state(ctrl, NVME_CTRL_FENCED) sets ctrl->state to
NVME_CTRL_FENCED, calls wake_up_all(&ctrl->state_wq), and drops ctrl->lock
before the second nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING) call:

CPU0                                  CPU1
nvme_tcp_fencing_work()               nvmf_subsystem_reset()
  nvme_change_ctrl_state(...,           nvme_wait_reset(ctrl)
    NVME_CTRL_FENCED)
    wake_up_all(&ctrl->state_wq)
                                          nvme_change_ctrl_state(...,
                                            NVME_CTRL_RESETTING) -> true
  nvme_change_ctrl_state(...,
    NVME_CTRL_RESETTING) -> false
  (err_work not queued)
                                        reg_write32() -> fails on dead sock
                                        returns error  <-- stuck in RESETTING

When nvmf_subsystem_reset() wins the transition, reg_write32() fails over
the broken transport before nvme_try_sched_reset(), leaving the controller
permanently wedged in NVME_CTRL_RESETTING with neither err_work nor
reset_work scheduled.  A concurrent nvme_reset_ctrl() (such as from
nvme_keep_alive_work(), nvme_anatt_timeout(), or sysfs reset_controller)
can likewise claim NVME_CTRL_RESETTING here and queue reset_work instead
of err_work, which attempts nvme_disable_ctrl() over the dead admin socket
and leaves the queues quiesced if nvme_tcp_setup_ctrl() fails.

[ ... ]

> @@ -2842,6 +2865,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
>  
>  static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl)
>  {
> +	flush_work(&to_tcp_ctrl(ctrl)->fencing_work);
>  	flush_work(&to_tcp_ctrl(ctrl)->err_work);
>  	cancel_delayed_work_sync(&to_tcp_ctrl(ctrl)->connect_work);
>  }

[Severity: High]
Can nvme_tcp_stop_ctrl() run flush_work(&to_tcp_ctrl(ctrl)->fencing_work)
before nvme_tcp_error_recovery() calls queue_work(), allowing fencing_work
to run during and after controller deletion?

In nvme_do_delete_ctrl(), nvme_stop_ctrl() runs before
ctrl->ops->delete_ctrl() stops the sockets and queues, and
nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING) is permitted from
NVME_CTRL_FENCING:

CPU0                                  CPU1
nvme_tcp_error_recovery()             nvme_do_delete_ctrl()
  nvme_change_ctrl_state(...,
    NVME_CTRL_FENCING) -> true
                                        nvme_change_ctrl_state(...,
                                          NVME_CTRL_DELETING) -> true
                                        nvme_stop_ctrl()
                                          nvme_tcp_stop_ctrl()
                                            flush_work(&fencing_work)
  dev_warn(...)
  queue_work(nvme_wq, &fencing_work)
                                        ...
                                        nvme_free_ctrl()
                                          kfree(ctrl)
nvme_tcp_fencing_work()
  nvme_fence_ctrl(ctrl)               <-- use-after-free

Because flush_work() returns immediately when fencing_work has not been
queued yet, fencing_work is never flushed again before the controller is
freed.

[ ... ]

> @@ -2944,17 +2968,19 @@ static enum blk_eh_timer_return nvme_tcp_timeout(struct request *rq)
[ ... ]
>  	/*
>  	 * LIVE state should trigger the normal error recovery which will
> -	 * handle completing this request.
> +	 * handle completing this request. In FENCING state the request must
> +	 * not be completed before fencing has finished, so just rearm the
> +	 * timer and let the error recovery pick it up.
>  	 */
>  	nvme_tcp_error_recovery(ctrl);
>  	return BLK_EH_RESET_TIMER;

[Severity: High]
While nvme_tcp_timeout() holds timed-out requests during NVME_CTRL_FENCING,
does nvme_tcp_try_send() still complete in-flight requests immediately on
a socket send error before fencing finishes?

When a controller enters NVME_CTRL_FENCING, its queues and queue->io_work
remain active until fencing_work finishes and queues err_work.  If an
in-flight write request has already sent its command capsule (or received
an R2T) and sock_sendmsg() fails while sending H2C data,
nvme_tcp_try_send() fails and completes the request right away:

drivers/nvme/host/tcp.c:nvme_tcp_try_send() {
	...
	} else if (ret < 0) {
		dev_err(queue->ctrl->ctrl.device,
			"failed to send request %d\n", ret);
		nvme_tcp_fail_request(queue->request);
		nvme_tcp_done_send_req(queue);
	}
	...
}

nvme_tcp_fail_request() completes the request with
NVME_SC_HOST_PATH_ERROR, which triggers multipath failover while the
controller is still in NVME_CTRL_LIVE or NVME_CTRL_FENCING and before
nvme_fence_ctrl() has terminated the command on the target.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=10