Re: [PATCH v2 24/33] ibmvfc: register local nvme fc port after fabric login

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Blocking operations (GFP_KERNEL allocations and wait_for_completion) are executed from atomic context, causing a system panic.
- [High] An unconditional `put_device()` on the `ibmvfc_nvme_unregister()` failure path without clearing the local port pointer leads to double-puts and use-after-free.
- [High] The NVMe local port and device reference are not cleaned up during driver removal, leading to a resource leak and potential use-after-free.
- [High] Re-initializing the host without an explicit adapter reset leaks the active `nvme_local_port` and overwrites its pointer with `NULL`, breaking NVMe functionality.
--

--- Patch [24]: [PATCH v2 24/33] ibmvfc: register local nvme fc port after fabric login ---
commit 4eb644adc42ffc19ca6cdbdf0f3e3185c26ee9b7
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: register local nvme fc port after fabric login

This patch delays registering the local nvme fc port until after the fabric
login completes, allowing the client port ID to be populated from the fabric
login response.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -5210,6 +5210,8 @@ static void ibmvfc_fabric_login_nvme_done(struct ibmvfc_event *evt)
>  
>  	switch (mad_status) {
>  	case IBMVFC_MAD_SUCCESS:
> +		fc_host_port_id(vhost->host) = be64_to_cpu(rsp->nport_id);
> +		ibmvfc_nvme_register(vhost);

[Severity: Critical]
Can this sequence safely execute under the host lock?

The completion handler ibmvfc_fabric_login_nvme_done() is invoked via
the ibmvfc_locked_done() wrapper, which acquires the vhost->host->host_lock
spinlock and disables interrupts.

However, ibmvfc_nvme_register() calls into nvme_fc_register_localport()
which performs memory allocations using GFP_KERNEL. Could this lead to a
scheduling while atomic regression?

[Severity: High]
What happens to the local port during a fabric link bounce?

If the link bounces, ibmvfc_link_down() deletes targets but doesn't appear
to unregister the local port. When the adapter re-initializes and completes
fabric login, this will unconditionally call ibmvfc_nvme_register() again.

If nvme_fc_register_localport() detects the existing port and returns
-EEXIST, its error path sets the portptr to NULL.

Could this leak the original local port and prevent nvme from functioning
until the driver is reloaded?

>  		ibmvfc_dbg(vhost, "NVMe fabric login succeeded\n");
>  		break;

[ ... ]

> @@ -5653,6 +5656,7 @@ static void ibmvfc_npiv_logout_done(struct ibmvfc_event *evt)
>  	case IBMVFC_MAD_SUCCESS:
>  		if (list_empty(&vhost->crq.sent) &&
>  		    vhost->action == IBMVFC_HOST_ACTION_LOGO_WAIT) {
> +			ibmvfc_nvme_unregister(vhost);

[Severity: Critical]
Is it safe to wait for completion here?

Similar to the registration path, ibmvfc_npiv_logout_done() executes under
the host_lock spinlock with interrupts disabled.

ibmvfc_nvme_unregister() calls wait_for_completion() which sleeps. Can
blocking here while holding the spinlock cause a scheduling while atomic
regression?

>  			ibmvfc_init_host(vhost);
>  			return;
>  		}

[ ... ]

> @@ -5931,6 +5935,7 @@ static void ibmvfc_do_work(struct ibmvfc_host *vhost)
>  		list_splice_init(&vhost->purge, &purge);
>  		spin_unlock_irqrestore(vhost->host->host_lock, flags);
>  		ibmvfc_complete_purge(&purge);
> +		ibmvfc_nvme_unregister(vhost);

[Severity: High]
Should the driver removal path also unregister the nvme local port?

During device unbind or module removal, ibmvfc_remove() tears down the
adapter but doesn't seem to call ibmvfc_nvme_unregister().

If the port remains registered with the nvme fc transport layer, its
private pointer still points to the vhost structure. After vhost is freed,
could subsequent transport accesses lead to a use-after-free, while also
leaking the device reference acquired during registration?

>  		rc = ibmvfc_reset_crq(vhost);
>  
>  		spin_lock_irqsave(vhost->host->host_lock, flags);

[ ... ]

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> --- a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> @@ -165,9 +168,17 @@ void ibmvfc_nvme_unregister(struct ibmvfc_host *vhost)
>  		return;
>  
>  	if (vhost->nvme_local_port) {
> +		ibmvfc_log(vhost, 2, "unregister_localport: host-traddr=nn-0x%llx:pn-0x%llx on portID:%x\n",
> +			   vhost->nvme_local_port->node_name,
> +			   vhost->nvme_local_port->port_name,
> +			   vhost->nvme_local_port->port_id);
>  		init_completion(&vhost->nvme_delete_done);
>  		rc = nvme_fc_unregister_localport(vhost->nvme_local_port);
> -		if (!rc)
> +		if (!rc) {
>  			wait_for_completion(&vhost->nvme_delete_done);
> +		} else
> +			dev_err(vhost->dev, "Failed to unregister NVMe fc localport (%d)\n", rc);
> +
> +		put_device(vhost->dev);

[Severity: High]
If nvme_fc_unregister_localport() fails, vhost->nvme_local_port is not
set to NULL, but put_device() is still called unconditionally.

If ibmvfc_nvme_unregister() is called again later, such as during a
subsequent adapter reset loop, will it attempt to unregister the same port
and drop the device reference a second time? Could this sequence result
in a use-after-free on the device structure?

>  	}
>  }

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