Re: [PATCH v2 28/33] ibmvfc: implement LLDD callbacks for mapping nvme-fc queues

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing `nvme_fc_unregister_localport` during adapter teardown causes Use-After-Free.
- [High] TOCTOU race in `ibmvfc_nvme_create_queue` leading to divide-by-zero and NULL pointer dereference.
- [High] Stub implementations of `ls_req` and `fcp_io` return 0 without invoking completion callbacks, causing uninterruptible kernel hangs.
--

--- Patch [28]: [PATCH v2 28/33] ibmvfc: implement LLDD callbacks for mapping nvme-fc queues ---
commit 186f72e380294760659d725fac77194c4e72d82e
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: implement LLDD callbacks for mapping nvme-fc queues

Implement the NVMe-FC queue create and delete callbacks and map NVMe
controller queues onto ibmvfc hardware queues. Use qidx of NVMe controller
queue to map onto a ibmvfc_queue channel. Add a new ibmvfc_nvme_qhandle
struct that will be used as the opaque queue handle by the NVMe-FC layer
when issuing fcp IO.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> index 637d7d7015cf5..ecf9f4c0549c2 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c

[ ... ]

> +static int ibmvfc_nvme_create_queue(struct nvme_fc_local_port *lport, unsigned int qidx,
> +				    u16 qsize, void **handle)
> +{
> +	struct ibmvfc_host *vhost = lport->private;
> +	struct ibmvfc_nvme_qhandle *qhandle;
> +
> +	if (!vhost->nvme_scrqs.active_queues)
> +		return -ENODEV;
> +
> +	qhandle = kzalloc_obj(struct ibmvfc_nvme_qhandle);
> +	if (!qhandle)
> +		return -ENOMEM;
> +
> +	qhandle->cpu_id = raw_smp_processor_id();
> +	qhandle->qidx = qidx;
> +
> +	/* Admin and first IO queue are both mapped to index 0 */
> +	if (qidx)
> +		qhandle->index = (qidx - 1) % vhost->nvme_scrqs.active_queues;

[Severity: High]
Could this result in a divide-by-zero or NULL pointer dereference?

The vhost->nvme_scrqs.active_queues value is read locklessly at the start of
the function. Since kzalloc_obj() can sleep, if a concurrent teardown or reset
(such as ibmvfc_release_channels()) executes during this window, it could set
active_queues to 0 and scrqs to NULL.

When the thread resumes, it would divide by zero here.

> +	else
> +		qhandle->index = qidx;
> +
> +	qhandle->queue = &vhost->nvme_scrqs.scrqs[qhandle->index];

[Severity: High]
Will this lead to a use-after-free during adapter teardown?

When unloading the driver or hot-removing the adapter, ibmvfc_remove() frees
the memory backing vhost->nvme_scrqs.scrqs via ibmvfc_release_sub_crqs().

However, it appears nvme_fc_unregister_localport() is never called during this
teardown path. Without notifying the NVMe-FC transport layer, it retains active
references to the local port and its queues. Any subsequent background transport
operations could dereference the freed queues.

[ ... ]

> @@ -59,8 +93,8 @@ static void ibmvfc_nvme_fcp_abort(struct nvme_fc_local_port *lport,
>  static struct nvme_fc_port_template ibmvfc_nvme_fc_transport = {
>  	.localport_delete	= ibmvfc_nvme_localport_delete,
>  	.remoteport_delete	= ibmvfc_nvme_remoteport_delete,
> -	.create_queue		= NULL,
> -	.delete_queue		= NULL,
> +	.create_queue		= ibmvfc_nvme_create_queue,
> +	.delete_queue		= ibmvfc_nvme_delete_queue,
>  	.ls_req			= ibmvfc_nvme_ls_req,
>  	.ls_abort		= ibmvfc_nvme_ls_abort,
>  	.fcp_io			= ibmvfc_nvme_fcp_io,

[Severity: High]
Does returning 0 from the stub implementations of ibmvfc_nvme_ls_req() and
ibmvfc_nvme_fcp_io() cause uninterruptible kernel hangs?

The NVMe-FC transport layer calls the LLDD's ls_req and fcp_io functions.
Returning 0 signals that the request was successfully dispatched and the
LLDD will asynchronously invoke the req->done() callback.

For example, __nvme_fc_send_ls_req() issues a synchronous wait_for_completion()
with no timeout. Because the stub functions simply return 0 and never call
done(), the caller would block forever, leaving block layer I/Os permanently
outstanding.

If these are meant to be stubs, should they return an error code like
-EOPNOTSUPP or -EIO instead?

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