| Message ID | 20260903034201.112211-29-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:
- [High] Failure of dma_map_sg() is incorrectly treated as success, leading to potential data corruption.
--
commit fe977aa03b75003e40bf53c83a27104877ab66a8
Author: Damien Le Moal <dlemoal@kernel.org>
scsi: mvumi: use 16-bits defined sense codes
Refactor the Marvell UMI driver to use scsi_set_sense() 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.
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e70d336b4ab39..a5bd3258a3994 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -2067,7 +2067,7 @@ static unsigned char mvumi_build_frame(struct mvumi_hba *mhba,
> return 0;
>
> error:
> - scsi_build_sense(scmd, 0, ILLEGAL_REQUEST, 0x24, 0);
> + scsi_set_sense(scmd, 0, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB);
[Severity: High]
This isn't a bug introduced by this patch, but looking at how we reach this
error block, does the driver mishandle a dma_map_sg() failure?
In mvumi_build_frame(), we call mvumi_make_sgl(). If dma_map_sg() fails
inside mvumi_make_sgl(), it returns 0:
drivers/scsi/mvumi.c:mvumi_make_sgl() {
...
*sg_count = dma_map_sg(&mhba->pdev->dev, scsi_sglist(scmd), sgnum,
scmd->sc_data_direction);
if (*sg_count > mhba->max_sge) {
...
return -1;
}
...
return 0;
}
Because 0 is not greater than mhba->max_sge, the error check is skipped,
and mvumi_make_sgl() returns 0 (success) instead of -1.
Back in mvumi_build_frame(), this bypasses the goto error:
drivers/scsi/mvumi.c:mvumi_build_frame() {
...
if (scsi_bufflen(scmd)) {
if (mvumi_make_sgl(mhba, scmd, &pframe->payload[0],
&pframe->sg_counts))
goto error;
pframe->data_transfer_length = scsi_bufflen(scmd);
}
...
}
Will this cause the hardware to reuse stale DMA addresses from a previous
command frame since data_transfer_length is set but sg_counts is 0?
> return -1;
> }
>
On 9/3/26 5:41 AM, Damien Le Moal wrote: > Refactor the Marvell UMI driver to use scsi_set_sense() 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/mvumi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Hannes Reinecke <hare@kernel.org> Cheers, Hannes
diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c index e70d336b4ab3..a5bd3258a399 100644 --- a/drivers/scsi/mvumi.c +++ b/drivers/scsi/mvumi.c @@ -2067,7 +2067,7 @@ static unsigned char mvumi_build_frame(struct mvumi_hba *mhba, return 0; error: - scsi_build_sense(scmd, 0, ILLEGAL_REQUEST, 0x24, 0); + scsi_set_sense(scmd, 0, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB); return -1; }
Refactor the Marvell UMI driver to use scsi_set_sense() 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/mvumi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)