Re: [PATCH v2 25/33] ibmvfc: process NVMe/FC rports in work thread

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Uninitialized variable `rport` used in NVMe target cleanup loop, leading to random state machine stalls.
- [High] Data race on `tgt->nvme_remote_port` causes NVMe remote port memory leak and dangling pointer.
- [High] Use-After-Free of NVMe remote port during concurrent unregistration.
--

--- Patch [25]: [PATCH v2 25/33] ibmvfc: process NVMe/FC rports in work thread ---
commit 5ac14eea9ebcccbdd022d855ccc9eba7a87b95be
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: process NVMe/FC rports in work thread

This commit adds NVMe-specific remote-port handling to the rport worker thread.
It registers new NVMe remote ports, rescans existing ones, and manages remote
port deletion transitions within the same worker context used for SCSI targets.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 81d9229bf388a..ffb579816e847 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -5856,6 +5856,51 @@ static void ibmvfc_log_ae(struct ibmvfc_host *vhost, int events)
>  		fc_host_post_event(vhost->host, fc_get_event_number(), FCH_EVT_LINKUP, 0);
>  }
>  
> +/**
> + * ibmvfc_tgt_add_nvme_rport - Tell the FC transport about a new remote port
> + * @tgt:		ibmvfc target struct
> + *
> + **/
> +static void ibmvfc_tgt_add_nvme_rport(struct ibmvfc_target *tgt)
> +{
> +	struct ibmvfc_host *vhost = tgt->vhost;
> +	struct nvme_fc_remote_port *rport;
> +	unsigned long flags;
> +
> +	tgt_dbg(tgt, "Adding NVMe rport\n");
> +	ibmvfc_nvme_register_remoteport(tgt);
> +	spin_lock_irqsave(vhost->host->host_lock, flags);
> +	rport = tgt->nvme_remote_port;
> +
> +	if (rport && tgt->action == IBMVFC_TGT_ACTION_DEL_RPORT) {
> +		tgt_dbg(tgt, "Deleting NVMe rport\n");
> +		list_del(&tgt->queue);
> +		ibmvfc_set_tgt_action(tgt, IBMVFC_TGT_ACTION_DELETED_RPORT);
> +		spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +		ibmvfc_nvme_unregister_remoteport(tgt);
> +		timer_delete_sync(&tgt->timer);
> +		kref_put(&tgt->kref, ibmvfc_release_tgt);
> +		return;
> +	} else if (rport && tgt->action == IBMVFC_TGT_ACTION_DEL_AND_LOGOUT_RPORT) {
> +		tgt_dbg(tgt, "Deleting NVMe rport with outstanding I/O\n");
> +		ibmvfc_set_tgt_action(tgt, IBMVFC_TGT_ACTION_LOGOUT_DELETED_RPORT);
> +		tgt->init_retries = 0;
> +		spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +		ibmvfc_nvme_unregister_remoteport(tgt);
> +		return;
> +	} else if (rport && tgt->action == IBMVFC_TGT_ACTION_DELETED_RPORT) {

[Severity: High]
Does this sequence create a data race and memory leak of the NVMe remote port?

If ibmvfc_nvme_register_remoteport() updates tgt->nvme_remote_port without
holding vhost->host->host_lock:

CPU1 in ibmvfc_tgt_add_nvme_rport():
    ibmvfc_nvme_register_remoteport(tgt); 

CPU2 in ibmvfc_do_work():
    spin_lock_irqsave(...)
    nvme_rport = tgt->nvme_remote_port; 
    list_del(&tgt->queue);
    ibmvfc_set_tgt_action(tgt, IBMVFC_TGT_ACTION_DELETED_RPORT);
    spin_unlock_irqrestore(...)
    // skips unregister because nvme_rport was read as NULL

Then CPU1 resumes in ibmvfc_tgt_add_nvme_rport():
    spin_lock_irqsave(...)
    if (... tgt->action == IBMVFC_TGT_ACTION_DELETED_RPORT) {
        spin_unlock_irqrestore(...)
        return; 
    }

Could this permanently leak the port structure and leave a dangling pointer in
the NVMe transport by returning without unregistering the newly allocated port?

> +		spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +		return;
> +	}
> +
> +	if (rport) {
> +		tgt_dbg(tgt, "NVMe rport add succeeded\n");
> +		tgt->target_id = tgt->nvme_remote_port->port_id;
> +	} else
> +		tgt_dbg(tgt, "NVMe rport add failed\n");
> +	spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +}

[ ... ]

> @@ -6073,6 +6119,30 @@ static void ibmvfc_do_work(struct ibmvfc_host *vhost)
>  			}
>  		}
>  
> +		list_for_each_entry(tgt, &vhost->nvme_scrqs.targets, queue) {
> +			if (tgt->action == IBMVFC_TGT_ACTION_DEL_RPORT) {
> +				tgt_dbg(tgt, "Deleteing NVMe rport\n");
> +				nvme_rport = tgt->nvme_remote_port;
> +				list_del(&tgt->queue);
> +				ibmvfc_set_tgt_action(tgt, IBMVFC_TGT_ACTION_DELETED_RPORT);
> +				spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +				if (nvme_rport)
> +					ibmvfc_nvme_unregister_remoteport(tgt);
> +				timer_delete_sync(&tgt->timer);
> +				kref_put(&tgt->kref, ibmvfc_release_tgt);
> +				return;
> +			} else if (rport && tgt->action == IBMVFC_TGT_ACTION_DEL_AND_LOGOUT_RPORT) {

[Severity: High]
Is rport uninitialized or containing stale data here?

This loop iterates over nvme_scrqs.targets, but rport is the local variable
used for the SCSI targets loop earlier in the function. If the preceding SCSI
loop exits early or finds no targets, rport could evaluate to false based on
uninitialized stack memory.

Should this use nvme_rport instead of rport? Also, if the registration failed
and the port pointer is NULL, does gating this state transition on port
existence cause the target to stall in the state machine indefinitely?

> +				tgt_dbg(tgt, "Deleting NVMe rport with outstanding I/O\n");
> +				nvme_rport = tgt->nvme_remote_port;
> +				ibmvfc_set_tgt_action(tgt, IBMVFC_TGT_ACTION_LOGOUT_DELETED_RPORT);
> +				tgt->init_retries = 0;
> +				spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +				if (nvme_rport)
> +					ibmvfc_nvme_unregister_remoteport(tgt);
> +				return;
> +			}
> +		}
> +

[ ... ]

> @@ -6691,7 +6762,28 @@ static void ibmvfc_rport_add_thread(struct work_struct *work)
>  				break;
>  			}
>  		}
> -	} while(did_work);
> +
> +		list_for_each_entry(tgt, &vhost->nvme_scrqs.targets, queue) {
> +			if (tgt->add_rport) {
> +				did_work = 1;
> +				tgt->add_rport = 0;
> +				kref_get(&tgt->kref);
> +				nvme_rport = tgt->nvme_remote_port;
> +				if (!nvme_rport) {
> +					spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +					ibmvfc_tgt_add_nvme_rport(tgt);
> +				} else {
> +					spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +					if (IS_ENABLED(CONFIG_NVME_FC))
> +						nvme_fc_rescan_remoteport(nvme_rport);

[Severity: High]
Can nvme_rport be freed while nvme_fc_rescan_remoteport() is running?

Once host_lock is dropped, a concurrent thread executing ibmvfc_do_work() could
transition the target to IBMVFC_TGT_ACTION_DEL_RPORT and call
ibmvfc_nvme_unregister_remoteport(), which frees the remote port.

The legacy SCSI code in this thread pins the port using get_device(&rport->dev)
before dropping the lock. Should a similar reference be taken for the NVMe
remote port before calling rescan?

> +				}
> +
> +				kref_put(&tgt->kref, ibmvfc_release_tgt);
> +				spin_lock_irqsave(vhost->host->host_lock, flags);
> +				break;
> +			}
> +		}
> +	} while (did_work);

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