Re: [PATCH v2 33/33] ibmvfc: fail nvme-fc fcp-io and ls requests during transport reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] The local variable `shwqs` is used uninitialized in `ibmvfc_purge_requests` when `vhost->using_channels` is false, leading to undefined behavior, out-of-bounds array access, and potential kernel crashes.

Pre-existing issues:
- [High] Dereferencing an uninitialized or stale `tgt` pointer during NVMe FCP aborts because `evt->tgt` is never initialized for FCP requests.
- [High] NVMe FC IO requests return a SCSI-specific busy code (`SCSI_MLQUEUE_HOST_BUSY`) when the CRQ is closed, causing immediate IO failures instead of retries.
--

--- Patch [33]: [PATCH v2 33/33] ibmvfc: fail nvme-fc fcp-io and ls requests during transport reset ---
commit ffc022564293825c73fe8e34bd53f9d7f153f0e8
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: fail nvme-fc fcp-io and ls requests during transport reset

The current purge code for flushing outstanding commands during a
transport reset only deals with SCSI commands. Rename the
ibmvfc_scsi_eh_done completion handler to ibmvfc_vfc_eh_done and wire it
to correctly complete nvme fcp and ls commands when flushing the
inflight command list during a reset.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index bc975f632da21..dfa231b6cc478 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> -static void ibmvfc_scsi_eh_done(struct ibmvfc_event *evt)
> +static void ibmvfc_vfc_eh_done(struct ibmvfc_event *evt)
>  {
>  	struct scsi_cmnd *cmnd = evt->cmnd;
> +	struct nvmefc_ls_req *ls_req = evt->ls_req;
> +	struct nvmefc_fcp_req *fcp_req = evt->fcp_req;

[Severity: High]
This is a pre-existing issue, but does evt->tgt remain uninitialized for
FCP requests?

When an NVMe FCP command is submitted via ibmvfc_nvme_fcp_io(), evt->tgt
is not fully initialized. 

If an abort occurs, ibmvfc_init_fcp_abort() reads the uninitialized
or stale pointer from abt_evt->tgt:

ibmvfc_init_fcp_abort()
    struct ibmvfc_event *abt_evt = abort_req->private;
    struct ibmvfc_target *tgt = abt_evt->tgt;
    ...
    tmf->target_wwpn = cpu_to_be64(tgt->wwpn);

Could this lead to a null pointer dereference or use-after-free crash
when reading tgt->wwpn during standard NVMe timeout and abort handling?

>  	if (cmnd) {
>  		scsi_dma_unmap(cmnd);
>  		scsi_done(cmnd);
> +	} else if (fcp_req) {
> +		fcp_req->rcv_rsplen = 0;

[ ... ]

> @@ -1164,13 +1176,16 @@ static void ibmvfc_fail_request(struct ibmvfc_event *evt, int error_code)
>  static void ibmvfc_purge_requests(struct ibmvfc_host *vhost, int error_code)
>  {
>  	struct ibmvfc_event *evt, *pos;
> -	struct ibmvfc_queue *queues = vhost->scsi_scrqs.scrqs;
> +	struct ibmvfc_queue *scsi_q = vhost->scsi_scrqs.scrqs;
> +	struct ibmvfc_queue *nvme_q = vhost->nvme_scrqs.scrqs;
>  	unsigned long flags;
> -	int hwqs = 0;
> +	int shwqs, nhwqs = 0;
>  	int i;
>  
> -	if (vhost->using_channels)
> -		hwqs = vhost->scsi_scrqs.active_queues;
> +	if (vhost->using_channels) {
> +		shwqs = vhost->scsi_scrqs.active_queues;
> +		nhwqs = vhost->nvme_scrqs.active_queues;
> +	}

[ ... ]

> -	for (i = 0; i < hwqs; i++) {
> -		spin_lock_irqsave(queues[i].q_lock, flags);
> -		spin_lock(&queues[i].l_lock);
> -		list_for_each_entry_safe(evt, pos, &queues[i].sent, queue_list)
> +	for (i = 0; i < shwqs; i++) {

[Severity: Critical]
Does this leave shwqs uninitialized when vhost->using_channels is false?

The declaration 'int shwqs, nhwqs = 0;' only initializes nhwqs to zero.

If a transport reset occurs on a host without channel support, the loop
condition checking against shwqs might use uninitialized memory. Can this
cause an out-of-bounds access on the scsi_q array and result in a kernel
panic?

> +		spin_lock_irqsave(scsi_q[i].q_lock, flags);
> +		spin_lock(&scsi_q[i].l_lock);
> +		list_for_each_entry_safe(evt, pos, &scsi_q[i].sent, queue_list)
>  			ibmvfc_fail_request(evt, error_code);
> -		list_splice_init(&queues[i].sent, &vhost->purge);
> -		spin_unlock(&queues[i].l_lock);
> -		spin_unlock_irqrestore(queues[i].q_lock, flags);
> +		list_splice_init(&scsi_q[i].sent, &vhost->purge);
> +		spin_unlock(&scsi_q[i].l_lock);
> +		spin_unlock_irqrestore(scsi_q[i].q_lock, flags);
> +	}

[ ... ]

> @@ -1818,7 +1843,9 @@ int ibmvfc_send_event(struct ibmvfc_event *evt,

[Severity: High]
This is a pre-existing issue, but earlier in ibmvfc_send_event() the code
unconditionally returns SCSI_MLQUEUE_HOST_BUSY when the send_crq error
code is H_CLOSED:

ibmvfc_send_event()
    if (rc == H_CLOSED) {
        ...
        return SCSI_MLQUEUE_HOST_BUSY;
    }

Will this cause the NVMe FC transport layer to interpret the non-EBUSY
return value as a hard failure?

The NVMe transport might immediately fail the IO instead of gracefully
retrying it during transient transport disruptions like VIOS restarts or
Live Partition Mobility, potentially leading to filesystem corruption.

>  		dev_err(vhost->dev, "Send error (rc=%d)\n", rc);
>  		if (evt->cmnd) {
>  			evt->cmnd->result = DID_ERROR << 16;
> -			evt->done = ibmvfc_scsi_eh_done;
> +			evt->done = ibmvfc_vfc_eh_done;
> +		} else if (evt->fcp_req || evt->ls_req) {
> +			evt->done = ibmvfc_vfc_eh_done;
>  		} else {
>  			evt->xfer_iu->mad_common.status = cpu_to_be16(IBMVFC_MAD_CRQ_ERROR);
>  			evt->done = evt->_done;

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