| Message ID | 20260831020451.585944-31-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Use defined 16-bits ASC/ASCQ combinations | expand |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Pre-existing issues: - [Critical] Missing `SAM_STAT_CHECK_CONDITION` assignment causes sense data to be ignored on DMA failure, resulting in silent data corruption. -- commit 9055c36d0afa69c58f35f6c4cc3a76397be71645 Author: Damien Le Moal <dlemoal@kernel.org> scsi: ibmvscsi_tgt: use 16-bits defined sense codes This commit refactors the IBM virtual SCSI target driver to use scsi_set_sense_buffer() and defined sense code enums instead of hard-coded values. This improves code clarity and self-documentation without intending any functional changes. > 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: Critical] This is a pre-existing issue, but does this error path properly notify the initiator of the failure? When srp_transfer_data() fails in ibmvscsis_queue_data_in(), the sense buffer is built, but it appears se_cmd->scsi_status is never set to SAM_STAT_CHECK_CONDITION. Because the status remains 0 (SAM_STAT_GOOD), when srp_build_response() is subsequently called, the check for rsp->status evaluates to false: srp_build_response() ... rsp->status = ibmvscsis_fast_fail(vscsi, cmd); if (rsp->status) { ... if (se_cmd->scsi_sense_length && se_cmd->sense_buffer) { rsp->sense_data_len = cpu_to_be32(se_cmd->scsi_sense_length); ... This causes the built sense data to be skipped, returning a success response to the initiator despite the DMA transfer failing. Can this lead to silent data corruption where the initiator incorrectly assumes the read was successful?
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c index 61f682800765..e2af90b6da39 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);
Refactor the IBM virtual SCSI target driver to use scsi_set_sense_buffer() and replace all hard-coded additional sense codes and additional sense code qualifiers with the enum values defined in include/scsi/scsi_sense.h. This helps with code clarity as the sense codes being processed are easier to test and self-documented. No functional change intended. Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)