Re: [PATCH v2 30/33] ibmvfc: implement nvme-fc IO command submission callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] NVMe FCP requests are incorrectly processed as MAD requests in error and purge paths, causing uninitialized pointer dereferences and silent data corruption.
- [Critical] Heap buffer overflow in scatterlist mapping due to mismatch between driver-advertised maximum segments and the DMA pool block size.
- [High] The driver passes uninitialized or stale NVMe Completion Queue Entries (CQE) to the NVMe-FC transport when the adapter skips the ERSP DMA.
- [High] `ibmvfc_nvme_fcp_io` propagates SCSI midlayer error codes directly to the NVMe-FC core, causing NVMe I/O requests to permanently fail instead of being retried.
- [Medium] Truncating a 64-bit pointer to a 32-bit `cancel_key` risks collisions across multiple command queues.
--

--- Patch [30]: [PATCH v2 30/33] ibmvfc: implement nvme-fc IO command submission callback ---
commit 6afa68277a7b61e829a727b116180c19195e096c
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: implement nvme-fc IO command submission callback

This commit implements the NVMe-FC IO command submission callback for the
ibmvfc driver. It adds helpers to initialize commands, map scatterlists,
and submit the requests to the hardware queue.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> index cf72f0682297b..d31e7e9ea7a9e 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
[ ... ]
> +static void ibmvfc_nvme_done(struct ibmvfc_event *evt)
> +{
> +	struct ibmvfc_cmd *vfc_cmd = &evt->xfer_iu->cmd;
> +	struct nvmefc_fcp_req *fcp_req = evt->fcp_req;
> +	struct nvme_fc_ersp_iu *ersp = (struct nvme_fc_ersp_iu *)fcp_req->rspaddr;
> +	struct nvme_completion *cqe = &ersp->cqe;
> +	struct nvme_command *sqe = &((struct nvme_fc_cmd_iu *)fcp_req->cmdaddr)->sqe;
> +
> +	ibmvfc_dbg(evt->vhost, "fc_done: (%x:%x)\n", be16_to_cpu(vfc_cmd->status),
> +		   be16_to_cpu(vfc_cmd->error));
> +	ibmvfc_dbg(evt->vhost, "fc_done: cmdlen: %d, rsplen %d, payload_len %d\n",
> +		   fcp_req->cmdlen, fcp_req->rsplen, fcp_req->payload_length);
> +
> +	fcp_req->status = 0;
> +	if (!vfc_cmd->status) {
> +		fcp_req->rcv_rsplen = NVME_FC_SIZEOF_ZEROS_RSP;
> +		fcp_req->transferred_length = fcp_req->payload_length;
> +	} else if (be16_to_cpu(vfc_cmd->status) & IBMVFC_FC_NVME_STATUS) {
> +		fcp_req->rcv_rsplen = sizeof(struct nvme_fc_ersp_iu);
> +		fcp_req->transferred_length = be32_to_cpu(ersp->xfrd_len);
> +		if (be16_to_cpu(vfc_cmd->error) & IBMVFC_NVMS_VALID_NODMA_CQE)
> +			cqe->command_id = sqe->common.command_id;

[Severity: High]
Does this pass uninitialized or stale NVMe Completion Queue Entries (CQE) to
the NVMe-FC transport?

If the adapter skips the ERSP DMA (indicated by IBMVFC_NVMS_VALID_NODMA_CQE),
this forces the NVMe core to process the response buffer by setting
fcp_req->rcv_rsplen = sizeof(struct nvme_fc_ersp_iu). Since the host memory at
fcp_req->rspaddr is recycled and not zeroed by the NVMe core, could this cause
the NVMe core to process stale cqe->status and cqe->result data without
verifying that IBMVFC_NVMS_VALID_ERSP was set?

> +	} else {
> +		fcp_req->rcv_rsplen = 0;
> +		fcp_req->transferred_length = 0;
> +		fcp_req->status = NVME_SC_INTERNAL;
> +	}
> +
> +	fcp_req->done(fcp_req);
> +	ibmvfc_free_event(evt);
> +}
> +
> +static struct ibmvfc_cmd *ibmvfc_nvme_init_vfc_cmd(struct ibmvfc_event *evt,
> +						   struct nvme_fc_remote_port *rport,
> +						   struct nvmefc_fcp_req *fcp_req)
> +{
> +	struct ibmvfc_target *tgt = rport->private;
> +	struct ibmvfc_cmd *vfc_cmd = &evt->iu.cmd;
> +
> +	memset(vfc_cmd, 0, sizeof(*vfc_cmd));
> +
> +	vfc_cmd->resp.va = cpu_to_be64(fcp_req->rspdma);
> +	vfc_cmd->resp.len = cpu_to_be32(fcp_req->rsplen);
> +	vfc_cmd->frame_type = cpu_to_be32(IBMVFC_NVME_FCP_TYPE);
> +	vfc_cmd->flags |= cpu_to_be16(IBMVFC_NVMEOF_PROTOCOL);
> +	vfc_cmd->payload_len = cpu_to_be32(fcp_req->cmdlen);
> +	vfc_cmd->resp_len = cpu_to_be32(fcp_req->rsplen);
> +	vfc_cmd->cancel_key = cpu_to_be32((u64)evt);

[Severity: Medium]
Does truncating the 64-bit evt pointer to a 32-bit cancel_key risk collisions?

Since events are allocated from per-queue DMA pools, is it possible for two
different event queues to be mapped at virtual memory addresses separated by
exactly 4GB? This would make their lower 32 bits identical, risking duplicate
cancel keys across multiple command queues and potentially aborting the wrong
command.

> +	vfc_cmd->target_wwpn = cpu_to_be64(rport->port_name);
> +	vfc_cmd->tgt_scsi_id = cpu_to_be64(rport->port_id);
> +	vfc_cmd->assoc_id = cpu_to_be64(tgt->assoc_id);
> +
> +	memcpy(&vfc_cmd->v3nvme.iu, fcp_req->cmdaddr, fcp_req->cmdlen);
> +
> +	return vfc_cmd;
> +}
[ ... ]
> +static int ibmvfc_nvme_map_sg_data(struct nvmefc_fcp_req *fcp_req,
> +				    struct ibmvfc_event *evt,
> +				    struct ibmvfc_cmd *vfc_cmd)
> +{
> +	struct srp_direct_buf *data = &vfc_cmd->ioba;
> +	struct ibmvfc_host *vhost = evt->vhost;
> +
> +	if (!fcp_req->sg_cnt) {
> +		vfc_cmd->flags |= cpu_to_be16(IBMVFC_NO_MEM_DESC);
> +		return 0;
> +	}
> +
> +	if (fcp_req->io_dir == NVMEFC_FCP_WRITE)
> +		vfc_cmd->flags |= cpu_to_be16(IBMVFC_WRITE);
> +	else
> +		vfc_cmd->flags |= cpu_to_be16(IBMVFC_READ);
> +
> +	if (fcp_req->sg_cnt == 1) {
> +		ibmvfc_nvme_map_sg_list(fcp_req, data);
> +		return 0;
> +	}
> +
> +	vfc_cmd->flags |= cpu_to_be16(IBMVFC_SCATTERLIST);
> +
> +	if (!evt->ext_list) {
> +		evt->ext_list = dma_pool_alloc(vhost->sg_pool, GFP_ATOMIC,
> +					       &evt->ext_list_token);

[Severity: Critical]
Can this overflow the DMA pool buffer?

The vhost->sg_pool appears to hold a maximum of 128 items, but if the
transport advertises support for up to 1024 scatterlist segments
(.max_sgl_segments = 1024), ibmvfc_nvme_map_sg_list() will iterate up to 1024
times. If fcp_req->sg_cnt exceeds 128, won't this write past the end of the
allocated buffer and corrupt adjacent memory?

> +
> +		if (!evt->ext_list)
> +			return -ENOMEM;
> +	}
> +
> +	ibmvfc_nvme_map_sg_list(fcp_req, evt->ext_list);
> +
> +	data->va = cpu_to_be64(evt->ext_list_token);
> +	data->len = cpu_to_be32(fcp_req->sg_cnt * sizeof(struct srp_direct_buf));
> +	data->key = 0;
> +	return 0;
> +}
> +
>  static int ibmvfc_nvme_fcp_io(struct nvme_fc_local_port *lport,
>  			      struct nvme_fc_remote_port *rport,
>  			      void *hw_queue_handle,
>  			      struct nvmefc_fcp_req *fcp_req)
>  {
> -	return 0;
> +	struct ibmvfc_host *vhost = lport->private;
> +	struct ibmvfc_nvme_qhandle *qhandle = hw_queue_handle;
> +	struct ibmvfc_cmd *vfc_cmd;
> +	struct ibmvfc_event *evt;
> +	int rc;
> +
> +	ibmvfc_dbg(vhost, "nvme_fcp_io\n");
> +	evt = ibmvfc_get_event(qhandle->queue);
> +	if (!evt)
> +		return -EBUSY;
> +
> +	evt->hwq = qhandle->index;
> +	ibmvfc_dbg(vhost, "vfc-nvme-mq-%d\n", evt->hwq);
> +
> +	ibmvfc_init_event(evt, ibmvfc_nvme_done, IBMVFC_CMD_FORMAT);
> +	evt->fcp_req = fcp_req;
> +	fcp_req->private = evt;
> +
> +	vfc_cmd = ibmvfc_nvme_init_vfc_cmd(evt, rport, fcp_req);
> +
> +	vfc_cmd->correlation = cpu_to_be64((u64)evt);
> +
> +	if (likely(!(rc = ibmvfc_nvme_map_sg_data(fcp_req, evt, vfc_cmd))))
> +		return ibmvfc_send_event(evt, vhost, 0);

[Severity: Critical]
Could this lead to NVMe FCP requests being incorrectly processed as MAD
requests in error paths?

If ibmvfc_send_event() fails (e.g., due to a transport error) or if
ibmvfc_fail_request() is called during a queue purge, does the driver assume
that any event without a SCSI cmnd is a MAD request? Since NVMe FCP requests
lack a cmnd, it appears they might fall into legacy branches where an
uninitialized evt->_done is executed, or where mad_common.status (which
overlaps with NVMe's payload_len) is written to instead of vfc_cmd->status,
resulting in a crash or the request being completed as a false success.

[Severity: High]
Will this propagate SCSI midlayer error codes directly to the NVMe-FC core?

If ibmvfc_send_event() detects a transient queue closure, it returns
SCSI_MLQUEUE_HOST_BUSY. Since the NVMe-FC core expects standard Linux error
codes (like -EBUSY) from the fcp_io callback, will returning this positive
error code cause it to be translated to BLK_STS_IOERR, leading to spurious
hard failures instead of retrying the I/O?

> +
> +	ibmvfc_free_event(evt);
> +
> +	return rc;
>  }

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