Re: [PATCH 04/20] scsi: ibmvfc: defer NVMe local port registration out of atomic context

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: ibmvfc: TOCTOU race in ibmvfc_do_work() clobbers asynchronous host state
- [Medium] scsi: ibmvfc: do_nvme_register flag leak causes spurious registrations
--

commit 93d3c68da91aa5e986f84439b6fba09a70dc93a4
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

scsi: ibmvfc: defer NVMe local port registration out of atomic context

This commit defers the NVMe local port registration to a kernel thread
context to avoid a scheduling while atomic panic. It accomplishes this
by introducing a do_nvme_register flag to indicate when registration
should occur in the ibmvfc_do_work() routine.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 26a73db662545..420ddb9584660 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -6072,6 +6072,12 @@ static void ibmvfc_do_work(struct ibmvfc_host *vhost)
>  			vhost->job_step(vhost);
>  		break;
>  	case IBMVFC_HOST_ACTION_QUERY:
> +		if (vhost->do_nvme_register) {
> +			vhost->do_nvme_register = 0;
> +			spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> +			ibmvfc_nvme_register(vhost);
> +			spin_lock_irqsave(&vhost->host->host_lock, flags);
> +		}

[Severity: High]
Does dropping the host_lock here open a window for asynchronous state
changes to be clobbered?

If the SCSI error handler triggers a host reset while the lock is dropped
(changing vhost->action to IBMVFC_HOST_ACTION_LOGO), won't reacquiring
the lock and blindly continuing down the IBMVFC_HOST_ACTION_QUERY path
overwrite the reset request?

Could this lead to a wedged SCSI host by permanently stalling error
recovery?

>  		list_for_each_entry(tgt, &vhost->scsi_scrqs.targets, queue)
>  			ibmvfc_init_tgt(tgt, ibmvfc_tgt_query_target);
>  		list_for_each_entry(tgt, &vhost->nvme_scrqs.targets, queue)
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
> index ca80ceffe53a5..df0775183d726 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.h
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.h
> @@ -1005,6 +1005,7 @@ struct ibmvfc_host {
>  	unsigned int nvme_enabled:1;
>  	unsigned int do_scsi_login:1;
>  	unsigned int do_nvme_login:1;
> +	unsigned int do_nvme_register:1;

[Severity: Medium]
Does this flag need to be explicitly cleared during adapter resets?

Looking at ibmvfc_reset_crq() and ibmvfc_reenable_crq_queue(), the other
state flags are cleared:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_reset_crq() {
    ...
    vhost->do_scsi_login = 0;
    vhost->do_nvme_login = 0;
    ...
}

If a reset occurs after this flag is set to 1 but before ibmvfc_do_work()
processes it, the stale flag seems to survive. Could this force a spurious
NVMe localport registration on reconnect even if the new fabric login did
not negotiate NVMe capabilities?

>  	unsigned int aborting_passthru:1;
>  	unsigned int scan_complete:1;
>  	int scan_timeout;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916230921.2204729-1-tyreld@linux.ibm.com?part=4