| Message ID | 20250105213618.531691-1-daniil.stas@posteo.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hwmon: drivetemp: Fix driver producing garbage data when SCSI errors occur | expand |
On 1/5/25 13:36, Daniil Stas wrote: > scsi_execute_cmd() function can return both negative (linux codes) and > positive (scsi_cmnd result field) error codes. > > Currently the driver just passes error codes of scsi_execute_cmd() to > hwmon core, which is incorrect because hwmon only checks for negative > error codes. This leads to hwmon reporting uninitialized data to > userspace in case of SCSI errors (for example if the disk drive was > disconnected). > > This patch checks scsi_execute_cmd() output and returns -EIO if it's > error code is positive. > > Signed-off-by: Daniil Stas <daniil.stas@posteo.net> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Chris Healy <cphealy@gmail.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Martin K. Petersen <martin.petersen@oracle.com> > Cc: Bart Van Assche <bvanassche@acm.org> > Cc: linux-kernel@vger.kernel.org > Cc: linux-scsi@vger.kernel.org > Cc: linux-ide@vger.kernel.org > Cc: linux-hwmon@vger.kernel.org > --- > > Although I see there is scsi_status_is_good() function, which probably > means that not all scsi result codes are errors? I don't know scsi > protocol much, so maybe someone else can check it. > The error code that i see when the drive is physically disconnected: 0x00030000. > Unless I am missing something, scsi_status_is_good() returns true for 0x00030000, so using that would miss this and various other errors. As far as I can see from the code, any non-zero host byte indicates an error, and the host byte is independent of the status (in the lower 16 bit). Guenter > drivers/hwmon/drivetemp.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c > index 6bdd21aa005a..fdf1d3b3b5a5 100644 > --- a/drivers/hwmon/drivetemp.c > +++ b/drivers/hwmon/drivetemp.c > @@ -192,8 +192,11 @@ static int drivetemp_scsi_command(struct drivetemp_data *st, > scsi_cmd[12] = lba_high; > scsi_cmd[14] = ata_command; > > - return scsi_execute_cmd(st->sdev, scsi_cmd, op, st->smartdata, > - ATA_SECT_SIZE, HZ, 5, NULL); > + int err = scsi_execute_cmd(st->sdev, scsi_cmd, op, st->smartdata, > + ATA_SECT_SIZE, HZ, 5, NULL); > + if (err > 0) > + err = -EIO; > + return err; > } > > static int drivetemp_ata_command(struct drivetemp_data *st, u8 feature,
On Sun, Jan 05, 2025 at 09:36:18PM +0000, Daniil Stas wrote: > scsi_execute_cmd() function can return both negative (linux codes) and > positive (scsi_cmnd result field) error codes. > > Currently the driver just passes error codes of scsi_execute_cmd() to > hwmon core, which is incorrect because hwmon only checks for negative > error codes. This leads to hwmon reporting uninitialized data to > userspace in case of SCSI errors (for example if the disk drive was > disconnected). > > This patch checks scsi_execute_cmd() output and returns -EIO if it's > error code is positive. > Applied. Thanks, Guenter
On Mon, 6 Jan 2025 08:43:54 -0800 Guenter Roeck <linux@roeck-us.net> wrote: > On Sun, Jan 05, 2025 at 09:36:18PM +0000, Daniil Stas wrote: > > scsi_execute_cmd() function can return both negative (linux codes) > > and positive (scsi_cmnd result field) error codes. > > > > Currently the driver just passes error codes of scsi_execute_cmd() > > to hwmon core, which is incorrect because hwmon only checks for > > negative error codes. This leads to hwmon reporting uninitialized > > data to userspace in case of SCSI errors (for example if the disk > > drive was disconnected). > > > > This patch checks scsi_execute_cmd() output and returns -EIO if it's > > error code is positive. > > > > Applied. > > Thanks, > Guenter Thanks!
diff --git a/drivers/hwmon/drivetemp.c b/drivers/hwmon/drivetemp.c index 6bdd21aa005a..fdf1d3b3b5a5 100644 --- a/drivers/hwmon/drivetemp.c +++ b/drivers/hwmon/drivetemp.c @@ -192,8 +192,11 @@ static int drivetemp_scsi_command(struct drivetemp_data *st, scsi_cmd[12] = lba_high; scsi_cmd[14] = ata_command; - return scsi_execute_cmd(st->sdev, scsi_cmd, op, st->smartdata, - ATA_SECT_SIZE, HZ, 5, NULL); + int err = scsi_execute_cmd(st->sdev, scsi_cmd, op, st->smartdata, + ATA_SECT_SIZE, HZ, 5, NULL); + if (err > 0) + err = -EIO; + return err; } static int drivetemp_ata_command(struct drivetemp_data *st, u8 feature,
scsi_execute_cmd() function can return both negative (linux codes) and positive (scsi_cmnd result field) error codes. Currently the driver just passes error codes of scsi_execute_cmd() to hwmon core, which is incorrect because hwmon only checks for negative error codes. This leads to hwmon reporting uninitialized data to userspace in case of SCSI errors (for example if the disk drive was disconnected). This patch checks scsi_execute_cmd() output and returns -EIO if it's error code is positive. Signed-off-by: Daniil Stas <daniil.stas@posteo.net> Cc: Guenter Roeck <linux@roeck-us.net> Cc: Chris Healy <cphealy@gmail.com> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Martin K. Petersen <martin.petersen@oracle.com> Cc: Bart Van Assche <bvanassche@acm.org> Cc: linux-kernel@vger.kernel.org Cc: linux-scsi@vger.kernel.org Cc: linux-ide@vger.kernel.org Cc: linux-hwmon@vger.kernel.org --- Although I see there is scsi_status_is_good() function, which probably means that not all scsi result codes are errors? I don't know scsi protocol much, so maybe someone else can check it. The error code that i see when the drive is physically disconnected: 0x00030000. drivers/hwmon/drivetemp.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)