Re: [PATCH 02/20] scsi: ibmvfc: fix trace logging for NVMe FCP commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] scsi: ibmvfc: trace corruption due to uninitialized ring buffer fields
- [Medium] scsi: ibmvfc: incorrect xfer_len logged for NVMe commands
--

commit 1aea7045df7aa168a5c3d098c49f569b870a3fa5
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

scsi: ibmvfc: fix trace logging for NVMe FCP commands
    
This commit fixes trace logging for NVMe FCP commands in ibmvfc_trc_start()
and ibmvfc_trc_end(). It adds gating on evt->cmnd and evt->fcp_req to prevent
reading SCSI-specific fields from an NVMe IU, which was producing garbage
trace entries.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index aa7ae81df41b1..6c243b2a84d7a 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -257,11 +257,16 @@ static void ibmvfc_trc_start(struct ibmvfc_event *evt)
>  
>  	switch (entry->fmt) {
>  	case IBMVFC_CMD_FORMAT:
> -		entry->op_code = iu->cdb[0];
>  		entry->scsi_id = be64_to_cpu(vfc_cmd->tgt_scsi_id);
> -		entry->lun = scsilun_to_int(&iu->lun);
> -		entry->tmf_flags = iu->tmf_flags;
> -		entry->u.start.xfer_len = be32_to_cpu(iu->xfer_len);
> +		if (evt->cmnd) {
> +			entry->op_code = iu->cdb[0];
> +			entry->lun = scsilun_to_int(&iu->lun);
> +			entry->tmf_flags = iu->tmf_flags;
> +			entry->u.start.xfer_len = be32_to_cpu(iu->xfer_len);
> +		} else if (evt->fcp_req) {
> +			entry->op_code = vfc_cmd->v3nvme.iu.sqe.common.opcode;
> +			entry->u.start.xfer_len = be32_to_cpu(vfc_cmd->payload_len);

[Severity: Medium]
Does this regression log the command frame size instead of the intended data
transfer length for NVMe commands? It appears to be using
vfc_cmd->payload_len (initialized from fcp_req->cmdlen) rather than
fcp_req->payload_length.

[Severity: Medium]
Could this regression leak stale data from previous trace events in the
vhost->trace ring buffer? Since the ring buffer fields are not zeroed before
reuse, NVMe commands will bypass setting entry->lun and entry->tmf_flags
without clearing them.

Additionally, TMF and NVMe LS commands have neither evt->cmnd nor
evt->fcp_req, bypassing this logic entirely and skipping initialization of
all these fields in ibmvfc_trc_start().

> +		}
>  		break;
>  	case IBMVFC_MAD_FORMAT:

[ ... ]

> @@ -294,15 +299,19 @@ static void ibmvfc_trc_end(struct ibmvfc_event *evt)
>  
>  	switch (entry->fmt) {
>  	case IBMVFC_CMD_FORMAT:
> -		entry->op_code = iu->cdb[0];
>  		entry->scsi_id = be64_to_cpu(vfc_cmd->tgt_scsi_id);
> -		entry->lun = scsilun_to_int(&iu->lun);
> -		entry->tmf_flags = iu->tmf_flags;
>  		entry->u.end.status = be16_to_cpu(vfc_cmd->status);
>  		entry->u.end.error = be16_to_cpu(vfc_cmd->error);
> -		entry->u.end.fcp_rsp_flags = rsp->flags;
> -		entry->u.end.rsp_code = rsp->data.info.rsp_code;
> -		entry->u.end.scsi_status = rsp->scsi_status;
> +		if (evt->cmnd) {
> +			entry->op_code = iu->cdb[0];
> +			entry->lun = scsilun_to_int(&iu->lun);
> +			entry->tmf_flags = iu->tmf_flags;
> +			entry->u.end.fcp_rsp_flags = rsp->flags;
> +			entry->u.end.rsp_code = rsp->data.info.rsp_code;
> +			entry->u.end.scsi_status = rsp->scsi_status;
> +		} else if (evt->fcp_req) {
> +			entry->op_code = vfc_cmd->v3nvme.iu.sqe.common.opcode;

[Severity: Medium]
Does the same regression apply here during event completion tracing? It looks
like completion response fields could contain stale trace data for TMF, LS,
and NVMe FCP commands in ibmvfc_trc_end() since they are skipped without
being zeroed.

> +		}
>  		break;
>  	case IBMVFC_MAD_FORMAT:

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