Re: [PATCH v2 31/33] ibmvfc: implement nvme-fc LS abort handling callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Concurrent completion of an NVMe FC-LS request causes a Use-After-Free of the event structure and incorrect cancellation of an unrelated command.
- [High] Inverted capability check sets the IBMVFC_TMF_SUPPRESS_ABTS flag only when the adapter lacks the capability.
- [High] The CRQ event structure is leaked from its fixed-size pool if the host is not logged in when an abort is issued.
- [Low] The local variable `status` is used uninitialized if `ibmvfc_send_event()` fails, leading to uninitialized stack memory being logged.
--

--- Patch [31]: [PATCH v2 31/33] ibmvfc: implement nvme-fc LS abort handling callback ---
commit 508e0371279894532111408d7a22f80e0afd86a5
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.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> index d31e7e9ea7a9e..390410e44f0d8 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,
>  	return 0;
>  }
>  
> +static void ibmvfc_sync_nvme_completion(struct ibmvfc_event *evt)
> +{
> +	/* copy the response back */
> +	if (evt->sync_iu)
> +		*evt->sync_iu = *evt->xfer_iu;
> +
> +	complete(&evt->comp);
> +}
> +
> +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 abt_evt be freed concurrently if the original LS request receives a
hardware completion?

If a natural completion races with the abort timeout, ibmvfc_ls_req_done()
could free the event structure back to the active pool via
ibmvfc_free_event(). Does this blind dereference risk a use-after-free or
corrupting a newly allocated command?

> +	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: High]
Is this capability check logically inverted?

It appears to set the IBMVFC_TMF_SUPPRESS_ABTS flag only when the adapter
lacks support for it, which might cause the firmware to reject the abort
TMF MAD.

> +	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 allocated event structure if the host is not
logged in?

If evt is successfully allocated from the fixed-size pool but
vhost->logged_in is false, returning here without calling
ibmvfc_free_event() would permanently lose the event. Repeated
occurrences could exhaust the 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))
> +		goto out;
> +
> +	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 the local variable status used uninitialized here?

If ibmvfc_send_event() fails, the code jumps to out, skipping the
assignment to status. The uninitialized stack value would then be
printed to the debug log.

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

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