Re: [PATCH 21/29] ibmvfc: process NVMe/FC rports in work thread

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Deterministic NULL pointer dereference during NVMe remote port and local port unregistration.
- [High] NVMe targets marked for deletion are completely ignored by the main state machine, resulting in permanent resource leaks and stuck target states.
- [High] `ibmvfc_nvme_unregister` is dead code, permanently leaking the NVMe local port on module unload.

Pre-existing issues:
- [Critical] Concurrent target deletion during port addition completely leaks the newly registered remote port, leading to a Use-After-Free of the private data pointer for NVMe targets.
- [High] Target structures allocated from a mempool are incorrectly freed using `kfree()`, draining the mempool reserves and leading to hangs under memory pressure.
--

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

ibmvfc: process NVMe/FC rports in work thread

This commit adds an NVMe-specific remote-port add helper and updates
the rport worker thread to walk the NVMe target list. It handles
registration, rescan, and deletion transitions for NVMe remote
ports in the same worker context used for SCSI rports.

> 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
[ ... ]
> +static void ibmvfc_tgt_add_nvme_rport(struct ibmvfc_target *tgt)
> +{
> +	struct ibmvfc_host *vhost = tgt->vhost;
[ ... ]
> +	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);

[Severity: Critical]
Does ibmvfc_nvme_unregister_remoteport() contain a null pointer dereference?

When unregistering the remote port, the completion callback appears to
explicitly set the tgt->nvme_remote_port pointer to NULL:

drivers/scsi/ibmvscsi/ibmvfc-nvme.c:ibmvfc_nvme_unregister_remoteport() {
    ...
    rc = nvme_fc_unregister_remoteport(tgt->nvme_remote_port);
    if (!rc) {
        wait_for_completion(&tgt->nvme_delete_done);
        tgt->nvme_remote_port->private = NULL;
    }
}

Will this attempt to dereference tgt->nvme_remote_port right after it was
set to NULL by ibmvfc_nvme_remoteport_delete before the completion signaled?
The same pattern seems to exist in ibmvfc_nvme_unregister() for the local
port.

> +		timer_delete_sync(&tgt->timer);
> +		kref_put(&tgt->kref, ibmvfc_release_tgt);

[Severity: High]
This is a pre-existing issue, but does ibmvfc_release_tgt() incorrectly free
mempool-allocated targets?

The targets are allocated via mempool_alloc(vhost->tgt_pool, GFP_NOIO) in
ibmvfc_alloc_targets(), but the kref release function calls kfree(tgt):

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_release_tgt() {
    struct ibmvfc_target *tgt = container_of(kref, struct ibmvfc_target, kref);
    kfree(tgt);
}

Does this bypass the mempool reserves and eventually cause mempool_alloc()
to hang under memory pressure?

> +		return;
> +	} else if (rport && tgt->action == IBMVFC_TGT_ACTION_DEL_AND_LOGOUT_RPORT) {
[ ... ]
> +	} else if (rport && tgt->action == IBMVFC_TGT_ACTION_DELETED_RPORT) {
> +		spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +		return;
> +	}

[Severity: Critical]
This is a pre-existing issue, but can this early return leak the newly
registered remote port?

If tgt->action is transitioned to IBMVFC_TGT_ACTION_DELETED_RPORT by the
state machine while the worker thread drops the lock to register the port,
we return early here without unregistering the newly created port.

For NVMe targets, tgt->nvme_remote_port->private points to the tgt
structure. If the tgt structure is subsequently freed via kref_put(), could
this cause a use-after-free in the NVMe midlayer when it uses the dangling
private data pointer?

[ ... ]
> @@ -6630,6 +6675,7 @@ static void ibmvfc_rport_add_thread(struct work_struct *work)
[ ... ]
> -	} while(did_work);
> +
> +		list_for_each_entry(tgt, &vhost->nvme_scrqs.targets, queue) {
> +			if (tgt->add_rport) {

[Severity: High]
Are NVMe targets marked for deletion completely ignored by the main state
machine?

The normal deletion path clears tgt->add_rport, which causes this worker
thread to ignore them. However, looking at the main state machine in
ibmvfc_do_work(), the deletion loop exists only for SCSI targets:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_do_work() {
    ...
    list_for_each_entry(tgt, &vhost->scsi_scrqs.targets, queue) {
        if (tgt->action == IBMVFC_TGT_ACTION_DEL_RPORT) {
            ...
}

Since the equivalent loop is omitted for nvme_scrqs.targets, do targets
marked for deletion remain unlinked and permanently leaked?

[Severity: High]
On a related note, does the driver fail to unregister the NVMe local port on
module unload?

The teardown path in ibmvfc_remove() removes the SCSI host via
scsi_remove_host() but does not appear to invoke the NVMe equivalent logic.
There is an ibmvfc_nvme_unregister() function provided in the code, but it
seems to be completely dead code that is never called.

Could this lead to a permanent resource leak and potential use-after-free
in the NVMe midlayer if it interacts with the removed host?

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