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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] nvme-fc: race between nvme_fc_start_ioerr_recovery() and
  controller delete
--

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

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

This commit transitions LIVE NVMe/FC controllers to FENCING on I/O errors
and runs Cross-Controller Reset (CCR) via fencing_work before proceeding to
RESETTING.

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 0ba621649afed..095adc9011ccd 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
[ ... ]
> @@ -1921,11 +1949,20 @@ char *nvme_fc_io_getuuid(struct nvmefc_fcp_req *req)
>  EXPORT_SYMBOL_GPL(nvme_fc_io_getuuid);
>  
>  /*
> - * Start recovery only if this caller can claim it with a state change.
> + * Start recovery only if this caller can claim it with a state change:
> + * FENCING from LIVE, or RESETTING otherwise.
>   */
>  static void
>  nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
>  {
> +	if (nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_FENCING)) {
> +		dev_warn(ctrl->ctrl.device,
> +			 "NVME-FC{%d}: starting controller fencing %s\n",
> +			 ctrl->cnum, errmsg);
> +		queue_work(nvme_wq, &ctrl->fencing_work);
> +		return;
> +	}
> +
>  	if (nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING)) {
>  		dev_warn(ctrl->ctrl.device, "NVME-FC{%d}: starting error recovery %s\n",
>  			 ctrl->cnum, errmsg);
> @@ -2439,6 +2476,12 @@ nvme_fc_ctrl_get(struct nvme_fc_ctrl *ctrl)
>  	return kref_get_unless_zero(&ctrl->ref);
>  }
>  
> +static void
> +nvme_fc_stop_ctrl(struct nvme_ctrl *nctrl)
> +{
> +	flush_work(&to_fc_ctrl(nctrl)->fencing_work);
> +}

[Severity: Medium]
Can ctrl->fencing_work be queued after nvme_fc_stop_ctrl() has already
flushed it if controller deletion races with nvme_fc_start_ioerr_recovery()?

nvme_change_ctrl_state() allows transitioning from NVME_CTRL_FENCING to
NVME_CTRL_DELETING, and nvme_fc_start_ioerr_recovery() drops ctrl->ctrl.lock
before calling dev_warn() and queue_work():

CPU0                                  CPU1
nvme_fc_start_ioerr_recovery()
  nvme_change_ctrl_state(...,
    NVME_CTRL_FENCING)
                                      nvme_delete_ctrl()
                                        nvme_change_ctrl_state(...,
                                          NVME_CTRL_DELETING)
                                      nvme_do_delete_ctrl()
                                        nvme_stop_ctrl()
                                          nvme_fc_stop_ctrl()
                                            flush_work(&ctrl->fencing_work)
  dev_warn(...)
  queue_work(nvme_wq,
    &ctrl->fencing_work)
                                        ...
                                        nvme_uninit_ctrl()
                                          nvme_put_ctrl() -> frees ctrl
                                      nvme_fc_fencing_work() <-- use-after-free

Because flush_work() returns immediately when fencing_work has not been
queued yet, nothing in the remainder of nvme_do_delete_ctrl() stops
nvme_fc_fencing_work() from running concurrently with nvme_fc_delete_ctrl()
or after struct nvme_fc_ctrl is freed.

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