Re: [PATCH v7 03/17] nvmet: Implement CCR nvme command

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] nvmet: TOCTOU race between nvmet_ctrl_find_get_ccr() and
  nvmet_start_ctrl()
--

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

nvmet: Implement CCR nvme command

This commit implements the Cross-Controller Reset admin command for the
NVMe target, allowing a source controller to look up and trigger a fatal
error on an impacted controller matching the specified controller ID, ciu,
and cirn.

> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 5710ae53eb83a..3df9db454db9c 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
[ ... ]
> @@ -1629,6 +1630,78 @@ void nvmet_execute_keep_alive(struct nvmet_req *req)
>  	nvmet_req_complete(req, status);
>  }
>  
> +void nvmet_execute_cross_ctrl_reset(struct nvmet_req *req)
> +{
[ ... ]
> +	/* Find and get impacted controller */
> +	ictrl = nvmet_ctrl_find_get_ccr(sctrl->subsys, sctrl->hostnqn,
> +					cmd->ccr.ciu, cntlid,
> +					le64_to_cpu(cmd->ccr.cirn));
[ ... ]
> +out_unlock:
> +	mutex_unlock(&sctrl->lock);
> +	if (status == NVME_SC_SUCCESS)
> +		nvmet_ctrl_fatal_error(ictrl);
> +	nvmet_ctrl_put(ictrl);
> +out:
> +	nvmet_req_complete(req, status);
> +}
[ ... ]
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 9c9ad4d9b292d..86018e0fb1586 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
[ ... ]
> @@ -1432,6 +1446,7 @@ static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
>  	if (!nvmet_is_disc_subsys(ctrl->subsys)) {
>  		ctrl->ciu = ((u8)(ctrl->ciu + 1)) ? : 1;
>  		ctrl->cirn = get_random_u64() ? : 1;
> +		nvmet_ctrl_cleanup_ccrs(ctrl, false);
>  	}
>  	ctrl->csts = NVME_CSTS_RDY;
>  
> @@ -1537,6 +1552,42 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const char *subsysnqn,
>  	return ctrl;
>  }
>  
> +struct nvmet_ctrl *nvmet_ctrl_find_get_ccr(struct nvmet_subsys *subsys,
> +					   const char *hostnqn, u8 ciu,
> +					   u16 cntlid, u64 cirn)
> +{
> +	struct nvmet_ctrl *ctrl, *ictrl = NULL;
> +	bool found = false;
> +
> +	mutex_lock(&subsys->lock);
> +	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
> +		if (ctrl->cntlid != cntlid)
> +			continue;
> +		if (strncmp(ctrl->hostnqn, hostnqn, NVMF_NQN_SIZE))
> +			continue;
> +
> +		/*
> +		 * Enabling the controller regenerates ciu and cirn under
> +		 * ctrl->lock. Hold the lock so that a controller becoming
> +		 * ready concurrently is matched against either the old or
> +		 * the new instance, never a mix of both.
> +		 */
> +		mutex_lock(&ctrl->lock);
> +		if (ctrl->ciu == ciu && ctrl->cirn == cirn)
> +			found = true;
> +		mutex_unlock(&ctrl->lock);

[Severity: Medium]
Can nvmet_execute_cross_ctrl_reset() race with nvmet_start_ctrl() and
trigger a fatal error on a newly enabled controller instance?

nvmet_ctrl_find_get_ccr() drops ctrl->lock right after matching ctrl->ciu
and ctrl->cirn.  Before nvmet_execute_cross_ctrl_reset() calls
nvmet_ctrl_fatal_error(ictrl), a concurrent nvmet_update_cc() can enable
ictrl and advance ictrl->ciu and ictrl->cirn:

CPU0                              CPU1
nvmet_execute_cross_ctrl_reset()
  nvmet_ctrl_find_get_ccr()
    mutex_lock(&ictrl->lock)
    /* matches ciu and cirn */
    mutex_unlock(&ictrl->lock)
                                  nvmet_update_cc()
                                    mutex_lock(&ictrl->lock)
                                    nvmet_start_ctrl()
                                      ictrl->ciu = ...;
                                      ictrl->cirn = ...;
                                    mutex_unlock(&ictrl->lock)
  nvmet_ctrl_fatal_error(ictrl)   <-- kills the new instance

If the interleaving goes the other way and nvmet_ctrl_fatal_error(ictrl)
runs just before nvmet_start_ctrl(), nvmet_start_ctrl() overwrites
ictrl->csts with the ready state, clearing the fatal status bit while
ictrl->fatal_err_work is already queued.

In addition, when a controller is re-enabled in-place via
nvmet_start_ctrl() rather than freed in nvmet_ctrl_free(),
nvmet_start_ctrl() only cleans up entries on ctrl->ccr_list where ctrl was
the source controller, leaving pending entries on other controllers where
ctrl was the impacted controller active with a non-null ccr->ctrl pointer.

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