Re: [PATCH v2 07/33] ibmvfc: split NVMe support into separate source file and add transport stubs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Uninitialized pointer dereference in `ibmvfc_send_event` error handling for SCSI Task Management Functions (TMFs).
- [Low] The static variable `ibmvfc_nvme_fc_transport` is defined but never used, triggering a `-Wunused-variable` compiler warning.
--

--- Patch [7]: [PATCH v2 07/33] ibmvfc: split NVMe support into separate source file and add transport stubs ---
commit 98e0f82a0d3f6f97d6d1d88906eb468378039a58
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: split NVMe support into separate source file and add transport stubs

This patch renames ibmvfc.c to ibmvfc-core.c to begin decoupling protocols
from the core driver logic. It also introduces ibmvfc-nvme.c and
ibmvfc-nvme.h with empty callback stubs and an nvme_fc_port_template. No
functional NVMe/FC support is enabled yet.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> similarity index 99%
> rename from drivers/scsi/ibmvscsi/ibmvfc.c
> rename to drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

Looking at ibmvfc_send_event() in the newly renamed ibmvfc-core.c:

        } else {
                evt->xfer_iu->mad_common.status = cpu_to_be16(IBMVFC_MAD_CRQ_ERROR);
                evt->done = evt->_done;
        }

        evt->done(evt);

[Severity: Critical]
Could the error path here overwrite a properly initialized done callback with
an uninitialized _done callback?

If ibmvfc_send_event() encounters an error sending a SCSI Task Management
Function (TMF) command (where rc != H_CLOSED and rc != 0), this else branch
is taken because TMFs are initialized with a NULL evt->cmnd.

Since IBMVFC_CMD_FORMAT does not initialize evt->_done in ibmvfc_init_event(),
can this copy an uninitialized or stale pointer into evt->done and immediately
execute it?

TMF operations like Abort Task Set or Target Reset are typically triggered
during SCSI error recovery, so this might lead to a wild function pointer
dereference under SAN error conditions.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> new file mode 100644
> index 0000000000000..2ee1c64665d9c
> --- /dev/null
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c

[ ... ]

> +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)
> +{
> +}
> +
> +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,
> +	.ls_req			= ibmvfc_nvme_ls_req,
> +	.ls_abort		= ibmvfc_nvme_ls_abort,
> +	.fcp_io			= ibmvfc_nvme_fcp_io,
> +	.fcp_abort		= ibmvfc_nvme_fcp_abort,
> +	.map_queues		= NULL,
> +	.max_hw_queues		= IBMVFC_NVME_HW_QUEUES,
> +	.max_sgl_segments	= 1024,
> +	.max_dif_sgl_segments	= 64,
> +	.dma_boundary		= 0xFFFFFFFF,
> +	.local_priv_sz		= sizeof(struct ibmvfc_host *),
> +	.remote_priv_sz		= sizeof(struct ibmvfc_target *),
> +	.lsrqst_priv_sz		= sizeof(struct ibmvfc_event *),
> +	.fcprqst_priv_sz	= sizeof(struct ibmvfc_event *),
> +};

[Severity: Low]
Is this static struct unused?

It appears ibmvfc_nvme_fc_transport is defined here but never referenced
anywhere in the file, which might trigger a compiler warning for an unused
variable.

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