Re: [PATCH v6 30/40] scsi: ibmvscsi_tgt: use 16-bits defined sense codes

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] In `ibmvscsis_queue_data_in`, when an `srp_transfer_data` failure occurs, the driver populates a sense buffer to indicate `LU_COMMUNICATION_TIMEOUT` but fails to set `se_cmd->scsi_status = SAM_STAT_CHECK_CONDITION`. As a result, the subsequent call to `srp_build_response` receives a `SAM_STAT_GOOD` (0) status, which causes it to completely bypass the sense data block and return a success response to the initiator, silently discarding the data transfer error.
--

commit 5043a368074e156668da603aa4c348be519e5fb6
Author: Damien Le Moal <dlemoal@kernel.org>

scsi: ibmvscsi_tgt: use 16-bits defined sense codes

This commit replaces hard-coded SCSI sense codes and qualifiers with standard
enum values from include/scsi/scsi_sense.h in the IBM virtual SCSI target
driver. The change aims to improve code clarity and make the sense codes
self-documenting without altering existing functionality.

> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index 61f6828007654..e2af90b6da39b 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -3757,9 +3757,8 @@ static int ibmvscsis_queue_data_in(struct se_cmd *se_cmd)
>  		dev_err(&vscsi->dev, "srp_transfer_data failed: %d\n", rc);
>  		se_cmd->scsi_sense_length = 18;
>  		memset(se_cmd->sense_buffer, 0, se_cmd->scsi_sense_length);
> -		/* Logical Unit Communication Time-out asc/ascq = 0x0801 */
> -		scsi_build_sense_buffer(0, se_cmd->sense_buffer, MEDIUM_ERROR,
> -					0x08, 0x01);
> +		scsi_set_sense_buffer(0, se_cmd->sense_buffer, MEDIUM_ERROR,
> +				      LU_COMMUNICATION_TIMEOUT);
>  	}
>  
>  	srp_build_response(vscsi, cmd, &len);

[Severity: High]
This isn't a bug introduced by this patch, but in this error path where
srp_transfer_data() fails, the sense buffer is populated with a timeout error
but se_cmd->scsi_status is not updated to SAM_STAT_CHECK_CONDITION.

Does this cause the subsequent call to srp_build_response() to see a
SAM_STAT_GOOD (0) status?

Looking at srp_build_response(), it checks the status:

    rsp->status = ibmvscsis_fast_fail(vscsi, cmd);
    if (rsp->status) {
        ...
        if (se_cmd->scsi_sense_length && se_cmd->sense_buffer) {
            ...
            memcpy(data, se_cmd->sense_buffer, se_cmd->scsi_sense_length);
        }
    }

If rsp->status remains 0, the entire sense-copy block appears to be bypassed.
Could this result in silently discarding the data transfer error and returning
a success response to the initiator, causing it to consume uninitialized or
stale data?

Would it be appropriate to set se_cmd->scsi_status = SAM_STAT_CHECK_CONDITION
before calling srp_build_response() in this failure path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908090308.1085097-1-dlemoal@kernel.org?part=30