Re: [PATCH v2 32/33] ibmvfc: implement nvme-fc FCP abort callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Lockless access to the original event in the FCP abort handler leads to Use-After-Free and spurious aborts of innocent I/O requests.
- [Critical] Dereferencing the uninitialized `abt_evt->tgt` pointer in `ibmvfc_init_fcp_abort` will cause a guaranteed kernel panic.
- [High] The `fcp_abort` callback blocks synchronously waiting for the abort MAD to complete on the wire, stalling the block layer's timeout workqueue.
- [High] FCP abort hangs forever if the adapter resets while the abort MAD is in-flight because NVMe queues are not purged.
--

--- Patch [32]: [PATCH v2 32/33] ibmvfc: implement nvme-fc FCP abort callback ---
commit b7d2ac0c1d18f363d56507d99dd9acbcd4cd3058
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

    ibmvfc: implement nvme-fc FCP abort callback
    
    Implement the NVMe-FC FCP abort callback by issuing an NVMF cancel MAD
    on the same submission queue used by the original FCP request.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> index 390410e44f0d8..54f80b6265d45 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> @@ -374,11 +374,75 @@ static int ibmvfc_nvme_fcp_io(struct nvme_fc_local_port *lport,
>  	return rc;
>  }
>  
> +static void ibmvfc_init_fcp_abort(struct ibmvfc_event *evt,
> +				  struct nvmefc_fcp_req *abort_req)
> +{
> +	struct ibmvfc_tmf *tmf;
> +	struct ibmvfc_event *abt_evt = abort_req->private;
> +	struct ibmvfc_target *tgt = abt_evt->tgt;
> +
> +	tmf = &evt->iu.tmf;
> +	memset(tmf, 0, sizeof(*tmf));
> +	tmf->common.version = cpu_to_be32(2);
> +	tmf->common.opcode = cpu_to_be32(IBMVFC_NVMF_TMF_MAD);
> +	tmf->common.length = cpu_to_be16(sizeof(*tmf));
> +	tmf->flags = cpu_to_be32(IBMVFC_TMF_ABORT_TASK | IBMVFC_TMF_NVMF_ASSOC);
> +	tmf->cancel_key = cpu_to_be32((u64)abt_evt);
> +	tmf->my_cancel_key = cpu_to_be32((u64)evt);
> +	tmf->target_wwpn = cpu_to_be64(tgt->wwpn);

[Severity: Critical]
Does this dereference an uninitialized pointer?

When an FCP IO is submitted via ibmvfc_nvme_fcp_io(), the evt->tgt field is
never populated.

When this IO times out, the code above extracts abt_evt->tgt and immediately
dereferences it to assign tmf->target_wwpn.

Since the correct target pointer is passed into fcp_abort as rport->private,
should that be used here instead to avoid a panic?

> +	tmf->assoc_id = cpu_to_be64(tgt->assoc_id);
> +	tmf->task_tag = cpu_to_be64((u64)abt_evt);
> +
> +	init_completion(&evt->comp);
> +}
> +
> +static void ibmvfc_nvme_fcp_abort(struct nvme_fc_local_port *lport,
> +				  struct nvme_fc_remote_port *rport,
> +				  void *hw_queue_handle,
> +				  struct nvmefc_fcp_req *abort_req)
> +{
> +	struct ibmvfc_host *vhost = lport->private;
> +	struct ibmvfc_target *tgt = rport->private;
> +	struct ibmvfc_event *evt, *abt_evt = abort_req->private;

[Severity: Critical]
Can accessing abort_req->private locklessly race with a hardware completion?

If an IO completes in hardware at the same time as a block layer timeout,
ibmvfc_nvme_done() can unconditionally free the event:

ibmvfc_nvme_done()
  ibmvfc_free_event(evt);

If this event is immediately reallocated for a new IO request on another CPU,
would using this reused event as the task_tag and cancel_key cause the VIOS
to abort the newly allocated, unrelated IO?

> +	struct ibmvfc_queue *queue;
> +	union ibmvfc_iu rsp;
> +	unsigned long flags;
> +	u16 status = 0;
> +
> +	if (!abt_evt)
> +		return;

[ ... ]

> +	spin_unlock_irqrestore(queue->q_lock, flags);
> +
> +	wait_for_completion(&evt->comp);

[Severity: High]
Does waiting synchronously here stall the blk-mq timeout worker thread?

The fcp_abort callback executes in the timeout workqueue. By waiting
synchronously for the MAD to complete on the wire, it seems this could stall
the timeout worker for up to default_timeout seconds, preventing timeout
handling for all other requests.

[Severity: High]
Could this wait_for_completion() block indefinitely during an adapter reset?

If the abort MAD times out, ibmvfc_timeout() resets the host and calls
ibmvfc_purge_requests() to fail all pending IOs.

However, the purge logic only loops over SCSI queues:

ibmvfc_purge_requests() {
  struct ibmvfc_queue *queues = vhost->scsi_scrqs.scrqs;
  ...
  for (i = 0; i < hwqs; i++) {
    list_for_each_entry_safe(evt, pos, &queues[i].sent, queue_list)
  ...
}

Since the NVMe queues (vhost->nvme_scrqs.scrqs) are completely ignored,
the NVMe MADs are left hanging. Will this cause this completion to never
be signaled?

> +	status = be16_to_cpu(rsp.mad_common.status);
> +
> +	spin_lock_irqsave(queue->q_lock, flags);
> +	ibmvfc_free_event(evt);
> +out:
> +	spin_unlock_irqrestore(queue->q_lock, flags);
> +
> +	if (status)
> +		ibmvfc_dbg(vhost, "fcp_abort: cancel failed with rc=%x\n", status);
> +
> +	kref_put(&tgt->kref, ibmvfc_release_tgt);
> +}

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