Re: [PATCH v7 13/17] nvme-fc: perform error recovery directly from ioerr_work

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] nvme-fc: race between nvme_fc_ctrl_ioerr_work() and controller
  deletion
- [Low] nvme-fc: unused forward declaration of
  __nvme_fc_abort_outstanding_ios()
--

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

nvme-fc: perform error recovery directly from ioerr_work

This commit replaces nvme_fc_error_recovery() with direct association
teardown, controller stop, and reconnect scheduling inside
nvme_fc_ctrl_ioerr_work().

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index c4603be381445..0ba621649afed 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -229,6 +229,8 @@ static struct device *fc_udev_device;
>  static void nvme_fc_complete_rq(struct request *rq);
>  static void nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl,
>  					 char *errmsg);
> +static void __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl,
> +					    bool start_queues);

[Severity: Low]
This isn't a bug, but nothing calls __nvme_fc_abort_outstanding_ios()
before its definition now that nvme_fc_error_recovery() is removed, so this
forward declaration is unused.

[ ... ]

> @@ -1880,7 +1883,23 @@ nvme_fc_ctrl_ioerr_work(struct work_struct *work)
>  	struct nvme_fc_ctrl *ctrl =
>  			container_of(work, struct nvme_fc_ctrl, ioerr_work);
>  
> -	nvme_fc_error_recovery(ctrl, "transport detected io error");
> +	nvme_stop_keep_alive(&ctrl->ctrl);
> +	flush_work(&ctrl->ctrl.async_event_work);
> +
> +	/* will block while waiting for io to terminate */
> +	nvme_fc_delete_association(ctrl);
> +	nvme_stop_ctrl(&ctrl->ctrl);
> +
> +	/* Do not reconnect if controller is being deleted */
> +	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
> +		return;
> +
> +	if (ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE) {
> +		queue_delayed_work(nvme_wq, &ctrl->connect_work, 0);
> +		return;
> +	}
> +
> +	nvme_fc_reconnect_or_delete(ctrl, -ENOTCONN);
>  }

[Severity: High]
Can nvme_fc_ctrl_ioerr_work() race with controller deletion and re-arm
ctrl->connect_work after nvme_fc_delete_ctrl() has already canceled it?

Previously, error recovery called nvme_reset_ctrl(), and
nvme_do_delete_ctrl() flushed ctrl->reset_work before calling
nvme_stop_ctrl(), nvme_remove_namespaces(), and ctrl->ops->delete_ctrl().
Unlike rdma and tcp, nvme_fc_ctrl_ops does not implement .stop_ctrl to
flush ioerr_work, and nvme_fc_delete_ctrl() cancels connect_work before
canceling ioerr_work:

drivers/nvme/host/fc.c:nvme_fc_delete_ctrl() {
	...
	cancel_delayed_work_sync(&ctrl->connect_work);
	...
	nvme_fc_delete_association(ctrl);
	cancel_work_sync(&ctrl->ioerr_work);
	...
}

If controller deletion starts right after nvme_fc_ctrl_ioerr_work() moves
the controller to NVME_CTRL_CONNECTING, connect_work can be queued after
cancel_delayed_work_sync(&ctrl->connect_work) has already completed:

CPU0                                    CPU1
nvme_fc_ctrl_ioerr_work()
  ...
  nvme_change_ctrl_state(...,
    NVME_CTRL_CONNECTING)
                                        nvme_delete_ctrl()
                                          nvme_change_ctrl_state(...,
                                            NVME_CTRL_DELETING)
                                          ...
                                          nvme_do_delete_ctrl()
                                            ...
                                            nvme_fc_delete_ctrl()
                                              cancel_delayed_work_sync(
                                                &ctrl->connect_work)
                                              ...
                                              cancel_work_sync(
                                                &ctrl->ioerr_work)
  queue_delayed_work(nvme_wq,
    &ctrl->connect_work, 0)
                                              nvme_remove_io_tag_set()
                                              nvme_remove_admin_tag_set()
                                            nvme_uninit_ctrl()
                                        nvme_fc_connect_ctrl_work()
                                          nvme_fc_create_association()
                                            <-- use-after-free

This leaves connect_work running nvme_fc_create_association() while the
tag sets are torn down and struct nvme_fc_ctrl is freed.

In addition, because nvme_do_delete_ctrl() does not wait for ioerr_work
before calling nvme_remove_namespaces(), nvme_fc_delete_association() in
ioerr_work can run concurrently with nvme_remove_namespaces(), racing on
quiescing and unquiescing the I/O queues while scan_work is flushed and
hardware queues are deleted.

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