Re: [PATCH 29/29] 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 4 potential issue(s) to consider:

New issues:
- [Critical] Pooled event structures retain stale NVMe request pointers, leading to exploitable Use-After-Free and double completions when non-NVMe commands fail.
- [High] Target kref is permanently leaked when an NVMe LS request fails or is purged.

Pre-existing issues:
- [High] Returning SCSI_MLQUEUE_HOST_BUSY for NVMe requests on transport closure causes permanent IO failures instead of requeuing.
- [Medium] NVMe FCP requests are not purged during a transport reset, rendering the patch's FCP functionality dead code.
--

commit 88ce7ce7f11c11cfca700c6919820cf4d9865c8a
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.

> 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.

[Severity: Medium]
This isn't a bug introduced by this patch, but does the purge code actually
evaluate inflight NVMe FCP commands?

Looking at ibmvfc_purge_requests(), the loop iterates over vhost->crq and
vhost->scsi_scrqs.scrqs:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_purge_requests() {
    ...
    for (i = 0; i < hwqs; i++) {
        spin_lock_irqsave(queues[i].q_lock, flags);
    ...
}

It seems to entirely miss vhost->nvme_scrqs.scrqs where FCP requests are
queued. If so, inflight NVMe FCP commands might be orphaned during a reset,
making the new FCP completion handling unreachable.

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

[ ... ]

> @@ -1092,20 +1092,28 @@ void ibmvfc_free_event(struct ibmvfc_event *evt)
>  }
>  
>  /**
> - * ibmvfc_scsi_eh_done - EH done function for queuecommand commands
> + * ibmvfc_vfc_eh_done - EH done function for queued IO
>   * @evt:	ibmvfc event struct
>   *
> - * This function does not setup any error status, that must be done
> - * before this function gets called.
> + * This function does not setup any error status for scsi commands, that must be
> + * done before this function gets called.
>   **/
> -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;
>  
>  	if (cmnd) {
>  		scsi_dma_unmap(cmnd);
>  		scsi_done(cmnd);
> -	}
> +	} else if (fcp_req) {
> +		fcp_req->rcv_rsplen = 0;
> +		fcp_req->transferred_length = 0;
> +		fcp_req->status = NVME_SC_INTERNAL;
> +		fcp_req->done(fcp_req);
> +	} else if (ls_req)
> +		ls_req->done(ls_req, -EIO);

[Severity: High]
Does this error handler leak the target kref for LS requests?

In ibmvfc_nvme_ls_req(), a reference is taken via kref_get(&tgt->kref) which
is typically dropped in the success path by ibmvfc_ls_req_done().

Since this patch replaces the completion handler with ibmvfc_vfc_eh_done(),
the transport request is completed here but kref_put(&evt->tgt->kref, ...)
is never called, potentially leaving the target structure leaked.

>  
>  	ibmvfc_free_event(evt);
>  }
> @@ -1146,8 +1154,10 @@ static void ibmvfc_fail_request(struct ibmvfc_event *evt, int error_code)
>  	BUG_ON(!atomic_dec_and_test(&evt->active));
>  	if (evt->cmnd) {
>  		evt->cmnd->result = (error_code << 16);
> -		evt->done = ibmvfc_scsi_eh_done;
> -	} else
> +		evt->done = ibmvfc_vfc_eh_done;
> +	} else if (evt->fcp_req || evt->ls_req)
> +		evt->done = ibmvfc_vfc_eh_done;
> +	else

[Severity: Critical]
Can these checks be triggered by stale NVMe pointers on non-NVMe commands?

When ibmvfc_get_event() reallocates an event structure from the pool,
ibmvfc_init_event() doesn't clear evt->fcp_req or evt->ls_req to NULL:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_init_event() {
    ...
    evt->cmnd = NULL;
    evt->sync_iu = NULL;
    evt->eh_comp = NULL;
    ...
}

If a non-NVMe command (e.g., a MAD command) is allocated an event with stale
NVMe pointers and fails here, will it incorrectly replace the completion
handler with ibmvfc_vfc_eh_done() and later use-after-free the stale NVMe
pointers?

>  		evt->xfer_iu->mad_common.status = cpu_to_be16(IBMVFC_MAD_DRIVER_FAILED);
>  
>  	timer_delete(&evt->timer);

[ ... ]

> @@ -1816,8 +1826,10 @@ int ibmvfc_send_event(struct ibmvfc_event *evt,

[Severity: High]
This isn't a bug introduced by this patch, but earlier in ibmvfc_send_event(),
it hardcodes a return of SCSI_MLQUEUE_HOST_BUSY on H_CLOSED without
distinguishing between SCSI and NVMe commands:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_send_event() {
    ...
    if (rc == H_CLOSED) {
        ...
        return SCSI_MLQUEUE_HOST_BUSY;
    }
    ...
}

This value (0x1055) is propagated back up through ibmvfc_nvme_fcp_io()
directly to the NVMe FC transport layer. Since 0x1055 is neither 0 nor
-EBUSY, will the NVMe FC core treat it as a hard failure and abort the
request with BLK_STS_IOERR instead of handling the queue pressure
appropriately?

>  		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;
> -		} else
> +			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);

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