Re: [PATCH 27/29] ibmvfc: implement nvme-fc LS abort handling callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The allocated ibmvfc_event structure is leaked on early return if the virtual host is not logged in.
- [High] Recursive spinlock self-deadlock or Use-After-Free on `ibmvfc_send_event()` failure.
- [High] Unsynchronized access to a potentially freed event structure (`abt_evt`) via `ls_abort->private` leads to a Use-After-Free.
- [High] Event leak when `ibmvfc_send_event()` fails, bypassing the freeing logic.
- [Medium] Logic inversion defeats the `IBMVFC_CAN_SUPPRESS_ABTS` capability check.
- [Low] Uninitialized stack variable `status` is read and printed on the error path.
- [Low] Returning a SCSI midlayer error code (`SCSI_MLQUEUE_HOST_BUSY`) to the NVMe FC transport causes unintended API violations.
--

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

ibmvfc: implement nvme-fc LS abort handling callback

Implement the NVMe FC-LS abort callback by issuing an ibmvfc cancel MAD
to the VIOS for the outstanding link-service request.

Use the saved event pointer from the original FC-LS request to identify
the command to cancel, submit the cancel operation, and complete the
abort request based on the returned status.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> index bff469d0b47df..18e8657abc444 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c

[ ... ]

> @@ -159,10 +161,72 @@ static int ibmvfc_nvme_ls_req(struct nvme_fc_local_port *lport,

[ ... ]

> +static void ibmvfc_init_ls_abort(struct ibmvfc_event *evt, struct nvmefc_ls_req *ls_abort)
> +{
> +	struct ibmvfc_tmf *tmf;
> +	struct ibmvfc_event *abt_evt = ls_abort->private;
> +	struct ibmvfc_target *tgt = abt_evt->tgt;

[Severity: High]
Can this sequence actually occur? If the NVMe FC transport invokes the abort
callback concurrently with the normal completion of the LS request, could
ibmvfc_ls_req_done() complete the request and free abt_evt to the pool before
we access abt_evt->tgt here?

> +	struct ibmvfc_host *vhost = evt->vhost;
> +
> +	tmf = &evt->iu.tmf;
> +	memset(tmf, 0, sizeof(*tmf));
> +	tmf->common.version = cpu_to_be32(2);
> +	tmf->target_wwpn = cpu_to_be64(tgt->wwpn);
> +	tmf->common.opcode = cpu_to_be32(IBMVFC_NVMF_TMF_MAD);
> +	tmf->common.length = cpu_to_be16(sizeof(*tmf));
> +	if (vhost->state != IBMVFC_ACTIVE)
> +		if (!ibmvfc_check_caps(vhost, IBMVFC_CAN_SUPPRESS_ABTS))
> +			tmf->flags = cpu_to_be32(IBMVFC_TMF_SUPPRESS_ABTS);

[Severity: Medium]
Does this logic correctly evaluate the capability? The check evaluates to true
only when the adapter does not support suppressing ABTS, which appears to
set IBMVFC_TMF_SUPPRESS_ABTS on adapters that do not support it, and omits
it on adapters that do.

> +	tmf->cancel_key = cpu_to_be32((u64)abt_evt);
> +	tmf->my_cancel_key = cpu_to_be32((u64)evt);
> +	tmf->assoc_id = cpu_to_be64(tgt->assoc_id);
> +
> +	init_completion(&evt->comp);
> +}
> +
>  static void ibmvfc_nvme_ls_abort(struct nvme_fc_local_port *lport,
>  				struct nvme_fc_remote_port *rport,
>  				struct nvmefc_ls_req *ls_abort)
>  {
> +	struct ibmvfc_host *vhost = lport->private;
> +	struct ibmvfc_target *tgt = rport->private;
> +	struct ibmvfc_event *evt;
> +	union ibmvfc_iu rsp;
> +	unsigned long flags;
> +	u16 status;
> +
> +	evt = ibmvfc_get_event(&vhost->crq);
> +	if (!vhost->logged_in || !evt)
> +		return;

[Severity: High]
Does this code leak the event structure? If vhost->logged_in is false,
ibmvfc_get_event() might successfully allocate an event from the vhost->crq
pool, but returning immediately here leaves the allocated event unreleased.
Repeated abort attempts could permanently exhaust the event pool.

> +
> +	spin_lock_irqsave(vhost->host->host_lock, flags);
> +	kref_get(&tgt->kref);
> +	ibmvfc_init_event(evt, ibmvfc_sync_nvme_completion, IBMVFC_MAD_FORMAT);
> +	ibmvfc_init_ls_abort(evt, ls_abort);
> +	evt->sync_iu = &rsp;
> +
> +	if (ibmvfc_send_event(evt, vhost, default_timeout))

[Severity: High]
Can this lead to a recursive self-deadlock or a use-after-free?

If ibmvfc_send_event() fails, it invokes evt->done(evt) synchronously:

ibmvfc_send_event()
    if (rc)
        evt->done(evt);

If the event is fresh, evt->done remains ibmvfc_locked_done(), which attempts
to acquire vhost->host->host_lock. Since this lock is already held here before
calling ibmvfc_send_event(), will this self-deadlock?

Additionally, because ibmvfc_init_event() doesn't zero evt->ls_req or
evt->fcp_req, could recycled events contain stale pointers that cause a
use-after-free when ibmvfc_send_event() evaluates those fields and replaces
evt->done with ibmvfc_vfc_eh_done() on failure?

[Severity: Low]
On a related note regarding ibmvfc_send_event(), when the receive queue is
closed it returns SCSI_MLQUEUE_HOST_BUSY. In places like ibmvfc_nvme_fcp_io()
that directly return the output of ibmvfc_send_event(), does returning a
positive SCSI midlayer error code to the NVMe FC transport cause unintended
API violations? The NVMe FC transport typically expects 0 for success or
standard negative error codes.

> +		goto out;

[Severity: High]
Does this goto skip the event cleanup? Although ibmvfc_send_event()'s error
path executes evt->done, ibmvfc_sync_nvme_completion() only calls complete()
on the event wait structure, meaning the event is never explicitly freed and
could be permanently leaked.

> +
> +	spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +
> +	wait_for_completion(&evt->comp);
> +	status = be16_to_cpu(rsp.mad_common.status);
> +	spin_lock_irqsave(vhost->host->host_lock, flags);
> +	ibmvfc_free_event(evt);
> +out:
> +	spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +	ibmvfc_dbg(vhost, "ls_abort: cancel failed with rc=%x\n", status);

[Severity: Low]
Is status uninitialized when we reach the out label? If ibmvfc_send_event()
fails and triggers the goto, status is never assigned and might leak
uninitialized stack memory into the logs.

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

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