NAS-142826 / 27.0.0-BETA.1 / drivetemp: report SAS reference temperature as temp_max, read real limits from subpage 02h - #347
Conversation
drivetemp_retrieve_temp_log() declares the LOG SENSE data buffer on the stack: char buf[TEMP_LOG_PAGE_LEN]; and hands it straight to scsi_execute_cmd() as the data-in buffer (drivers/hwmon/drivetemp.c:340 and :348-349). Stack memory is not a legal DMA target. Documentation/core-api/dma-api-howto.rst:132-134 states: "This rule also means that you may use neither kernel image addresses (items in data/text/bss segments), nor module image addresses, nor stack addresses for DMA." With CONFIG_VMAP_STACK (default on x86_64, and set in the TrueNAS production config) the kernel stack is a vmalloc() mapping, which dma-api-howto.rst:125-126 calls out explicitly as unusable for DMA. Today this does not corrupt memory only because the block layer still carries a safety net: blk_rq_map_kern() routes the request through the copy path whenever object_is_on_stack(kbuf) is true (block/blk-map.c:774, object_is_on_stack() at include/linux/sched/task_stack.h:89). So every temperature poll silently allocates and copies a bounce buffer, and the driver's correctness rests on a bounce path the block layer is not obliged to keep. The rest of this driver does not rely on it: every ATA/SATA command DMAs into st->smartdata (drivers/hwmon/drivetemp.c:204- 205), a u8 smartdata[ATA_SECT_SIZE] member of the kzalloc()'d struct drivetemp_data (drivers/hwmon/drivetemp.c:118, allocated at :647). That is page-allocator-backed memory and a legal DMA target. Make the SCSI path do the same. ATA_SECT_SIZE is 512 (include/linux/ata.h:28) and TEMP_LOG_PAGE_LEN is 0x10 (drivers/hwmon/drivetemp.c:148), so the log page fits with room to spare, including the larger allocation length a later patch in this series needs for log page 0x0d subpage 0x02. Sharing st->smartdata is safe. The only runtime caller, drivetemp_get_scsitemp(), is reached through st->get_temp() under st->lock (drivers/hwmon/drivetemp.c:549-551), the mutex documented at :114 as protecting data buffer accesses. The only other caller, drivetemp_identify_scsi(), runs from drivetemp_add() at :655, before hwmon_device_register_with_info() at :660 publishes the device, so no sysfs reader can race it. Two consequences of the buffer now being persistent are handled here: - Zero the bytes we are about to parse before issuing the command. scsi_execute_cmd() already zeroes the residual tail on a short transfer (drivers/scsi/scsi_lib.c:341-342), but that depends on the LLDD reporting resid_len correctly; without the memset a driver that does not would leave the parser reading last poll's temperature, or leftover SATA SMART data, instead of obviously-invalid zeros. - Bound the temperature read. The parameter walk only checks i + param_len <= page_len, but reads the temperature at buf[i + TEMP_LOG_PARAM_TEMP_OFFSET] == buf[i + 5], which a device reporting a parameter shorter than six bytes does not cover. On the stack that was a small over-read of a 16-byte array, catchable by KASAN; with a shared 512-byte buffer it would instead return plausible-looking stale bytes. Skip parameters too short to contain the temperature byte. The buffer type changes from char to u8. The parser already cast every byte it consumed to u8 (:357, :362, :364) and get_unaligned_be16() takes a const void *, so no parsing behaviour changes; the two now-redundant casts on the temperature reads are dropped. The CDB array stays on the stack: scsi_execute_cmd() memcpy()s it into scmd->cmnd (drivers/scsi/scsi_lib.c:318-319) and never DMAs from it, which is why drivetemp_scsi_command() does the same at drivers/hwmon/drivetemp.c:174. No functional change intended for well-behaved drives. Fixes: 06ff843 ("hwmon: (drivetemp) Add SCSI drives support") Signed-off-by: Eric Schultz <eric@startuperic.com>
drivetemp_retrieve_temp_log() returns the raw return value of
scsi_execute_cmd() to its callers:
err = scsi_execute_cmd(st->sdev, scsi_cmd, REQ_OP_DRV_IN, buf,
TEMP_LOG_PAGE_LEN, 10 * HZ, 5, NULL);
if (err)
return (err);
scsi_execute_cmd() is documented at drivers/scsi/scsi_lib.c:287-288 as
returning "the scsi_cmnd result field if a command was executed, or a
negative Linux error code if we didn't get that far", and the
implementation does exactly that at drivers/scsi/scsi_lib.c:352
("ret = scmd->result;"). A drive that terminates the LOG SENSE with
CHECK CONDITION therefore produces a *positive* return value, not an
errno.
That positive value propagates unmodified through
drivetemp_get_scsitemp() (drivers/hwmon/drivetemp.c:377-383, which only
tests "== 0") into drivetemp_read() and out to the hwmon core.
hwmon_attr_show() only rejects negative returns:
drivers/hwmon/hwmon.c:427 long val;
drivers/hwmon/hwmon.c:430-433 ret = hattr->ops->read(...., &val);
if (ret < 0)
return ret;
so a positive result is treated as success and the uninitialised stack
variable 'val' is formatted and handed to userspace as the drive
temperature. A SAS drive that fails LOG SENSE (media error, unit
attention after a reset, an offline device, or a drive that does not
implement log page 0x0d) thus reports a garbage temp1_input instead of
failing the read.
The SATA sibling in this same file already normalises correctly, at
drivers/hwmon/drivetemp.c:204-208:
err = scsi_execute_cmd(st->sdev, scsi_cmd, op, st->smartdata,
ATA_SECT_SIZE, 10 * HZ, 5, NULL);
if (err > 0)
err = -EIO;
return err;
Apply the same normalisation on the SCSI path. Like the SATA path, this
deliberately treats *any* CHECK CONDITION as a failed read: a LOG SENSE
completing with sense key NO SENSE or RECOVERED ERROR transferred usable
data but will now return -EIO rather than a temperature. That is a
conscious trade for not returning uninitialised stack data, and it
matches the existing behaviour of every other command this driver
issues.
The secondary caller, drivetemp_identify_scsi()
(drivers/hwmon/drivetemp.c:387-400), was already benign because
drivetemp_add() collapses any non-zero identify result to -ENODEV
(drivers/hwmon/drivetemp.c:655-658), but it now also gets a sane errno.
No functional change for drives whose LOG SENSE succeeds.
Fixes: 06ff843 ("hwmon: (drivetemp) Add SCSI drives support")
Signed-off-by: Eric Schultz <eric@startuperic.com>
… temp_crit The SCSI path added in 43ff910 ("hwmon: (drivetemp) Add SCSI drives support") reads log page 0Dh (Temperature) and maps log parameter 0001h, REFERENCE TEMPERATURE, to temp1_crit (drivers/hwmon/drivetemp.c:396): if (reftemp != TEMP_LOG_INVALID) { st->have_temp_crit = true; st->temp_crit = reftemp * 1000; } That is the wrong tier. SPC-2 through SPC-5 define the reference temperature as "the maximum reported sensor temperature at which the SCSI target device is capable of operating continuously without degrading operation or reliability". The parameter is defined with ETC = 0, i.e. "no threshold comparison is made on this value", and the standard states explicitly that "no comparison is performed between the temperature value specified in parameter 0000h and the reference temperature specified in parameter 0001h". It is a recommended continuous-operation ceiling, not a damage threshold, and it is host-writable via LOG SELECT on several vendors' drives (Seagate documents a default of 65 C). The hwmon ABI reserves temp_crit for the damage tier: Documentation/hwmon/sysfs-interface.rst:254-256 `temp[1-*]_crit` Temperature critical max value, typically greater than corresponding temp_max values. and drivetemp's own documentation already draws the distinction in wording that matches the SPC definition almost verbatim: Documentation/hwmon/drivetemp.rst:64 temp1_max Maximum recommended continuous operating temperature Documentation/hwmon/drivetemp.rst:65 temp1_crit Maximum temperature limit. Operating the device above this temperature may cause physical damage to the device. The SATA path in this same file already tiers correctly, mapping the SCT data table's recommended maximum (byte 6) to temp_max and the Temperature Limit (byte 7) to temp_crit (drivers/hwmon/drivetemp.c:496-497, :501-502), so the SCSI path was inconsistent with the SATA path in the same driver. Other drivers agree: nvme maps WCTEMP to temp_max and CCTEMP to temp_crit (drivers/nvme/host/hwmon.c:79-85), and k10temp maps a nominal operating ceiling to temp_max and the HTC trip point to temp_crit (drivers/hwmon/k10temp.c:250-257). Concrete user-visible damage: a NETAPP-rebadged Seagate 4 TB SAS drive reports 40 C in parameter 0001h and idles at 46-47 C. With the current mapping the drive is permanently above its advertised temp1_crit while reporting SMART Health Status: OK, so any consumer of temp1_crit raises a permanent critical condition on a healthy drive. Move the value to the tier the standard puts it in. No other change is needed; the temp_max infrastructure is already present and is exercised today by the SATA path: - drivetemp_read() already services hwmon_temp_max from st->temp_max (drivers/hwmon/drivetemp.c:559-561); - drivetemp_is_visible() already gates temp1_max on st->have_temp_max (drivers/hwmon/drivetemp.c:595-598); - HWMON_T_MAX is already in drivetemp_info (drivers/hwmon/drivetemp.c:620-623). The driver declares no *_alarm attribute, so nothing derives an alarm bit from the moved value, and st->have_temp_crit has no reader other than drivetemp_is_visible() (drivers/hwmon/drivetemp.c:604). NOTE, because this is a userspace-visible ABI change and not merely an internal retier: a SAS drive that implements only log page 0Dh subpage 00h will, after this patch, expose temp1_input and temp1_max and *no temp1_crit at all*. The attribute disappears from /sys/class/hwmon/hwmonN/. That is the honest representation - such a drive has never told us a damage threshold - but consumers must be updated in the same release. In TrueNAS specifically, smartd was removed in 25.10 and alert/source/disk_temp.py is the only remaining disk temperature alert source; it skips any disk whose crit is None, so between this patch landing and the middleware fallback patch landing, SAS disks have *zero* thermal alerting rather than a false critical one. Ship them together, middleware first or simultaneously. Real critical limits for SAS come from log page 0Dh subpage 02h (Environmental limits), added in the next patch. Fixes: 06ff843 ("hwmon: (drivetemp) Add SCSI drives support") Signed-off-by: Eric Schultz <eric@startuperic.com>
…bpage 0x02 The SCSI path only ever reads log page 0x0d subpage 0x00. That page carries the current temperature (parameter 0x0000) and the reference temperature (parameter 0x0001), and nothing else; there is no damage threshold in it. Byte 3 of the LOG SENSE CDB (SUBPAGE CODE) was never written, so subpage 0 was the only thing the driver could ask for, and the 0x10 byte allocation length is too small for anything larger. SPC-5 added the Environmental limits subpage (page 0x0d, subpage 0x02). For each temperature parameter (parameter codes 0x0000-0x00ff; 0x0100-0x01ff are humidity parameters) it reports, at offsets from the start of the parameter: byte 4 high critical temperature limit trigger byte 5 high critical temperature limit reset byte 6 low critical temperature limit reset byte 7 low critical temperature limit trigger byte 8 high operating temperature limit trigger byte 9 high operating temperature limit reset (the layout sg3_utils' sg_logs show_environmental_limits_page() decodes). Probe that subpage and, when the drive implements it, expose the high critical limit trigger as temp1_crit and the high operating limit trigger as temp1_max. That is the tiering the hwmon ABI asks for (Documentation/hwmon/sysfs-interface.rst:254-256: "temp[1-*]_crit: Temperature critical max value, typically greater than corresponding temp_max values"), and it restores a real temp1_crit on SAS, which the previous patch removed when it moved the reference temperature to temp1_max. The two limits are used only if the drive reports both of them, both are plausible, and the critical limit is above the operating limit. A drive that implements the subpage but not an individual field reports that field as 0, which is exactly how a bogus 0 C "critical" threshold gets into userspace; 0 and 0xFF are therefore both rejected, the bytes are decoded as signed temperatures the same way the SATA path decodes SCT limits (temp_from_sct(), drivers/hwmon/drivetemp.c:159), and anything that fails those tests falls back wholesale to the reference temperature. The subpage is never allowed to contribute one tier and the reference temperature the other. Drives that predate SPC-5 terminate the command with ILLEGAL REQUEST. That is detected through struct scsi_exec_args.sshdr (include/scsi/scsi_device.h:540-548) and turned into -EOPNOTSUPP, and the caller falls back to publishing the reference temperature as temp1_max. Only the sense key is examined; the additional sense code is deliberately ignored, because although 0x24 (invalid field in CDB) is the expected response, firmware also returns 0x20 and 0x26, and the cost of matching broadly is only that a genuinely malformed request takes the same harmless fallback. -EOPNOTSUPP is confined to the probe: the plain temperature read collapses it back to -EIO so that a drive which starts rejecting page 0x0d mid-life keeps returning -EIO from temp1_input. Some drives ignore the subpage code entirely and answer with subpage 0x00. The SPF bit, the page code and the subpage code in the returned page header are all checked so that such a response is not misparsed as limits. The LOG SENSE issue is factored out into drivetemp_log_sense(), which takes the subpage and the allocation length and reports the length of the returned page. It keeps DMAing into st->smartdata; the new 0x80 byte allocation length is well within ATA_SECT_SIZE (512, include/linux/ata.h:28), the size of that buffer (drivers/hwmon/drivetemp.c:118), and the allocation length is bounds checked at both ends so that the page_len clamp cannot underflow. Only parameter 0x0000 is used, matching drivetemp_retrieve_temp_log(), so the limits always describe the same sensor that feeds temp1_input. Parameters shorter than 12 bytes (4 byte parameter header plus 8 bytes of data) do not reach the high operating limit trigger at offset 8 and are rejected. This costs one extra LOG SENSE per drive at probe time only; nothing is added to the per-poll path. Fixes: 06ff843 ("hwmon: (drivetemp) Add SCSI drives support") Signed-off-by: Eric Schultz <eric@startuperic.com>
bugclerk
changed the title
yocalebo
left a comment
yocalebo
left a comment
There was a problem hiding this comment.
Yeah....no 🙂 an insane amount of change that has profound impact on every enterprise customer we support. You'll need to submit this upstream for review from the maintainers first. Feel free to include us on the report if you want.
|
Fair point on scope, and I'll split this up. But I need to flag something I should have checked before opening it: there's nothing upstream to submit. The SCSI temperature path this series touches isn't in mainline. I'm glad to help get the underlying feature upstream if that's something iX wants — but that's Ameer's patch to send, and it's a much larger conversation than these fixes. In the meantime, two of the four carry no ABI change and no behaviour change on a healthy drive:
That second one is worth a look: upstream already fixed exactly this bug on the ATA path in 82163d6 (Daniil Stas, v6.13), which is where the I have those two on a separate branch off I'll leave this PR for the parts that actually deserve the scrutiny: the |
|
Sure, the bug fix looks legit and if it is already upstream then sure. I'm surprised the upstream fix is only upstream 6.13? They usually backport and apply the fixes across the releases so curious why 6.18 doesn't have it? Also, the SCSI kernel module temp was written specifically for our enterprise use case. I'm not sure we're interested in changing it since the jira ticket specifically quotes NetApp rebranded seagate drives. We can't support every incantation of esoteric hardware combination out there which is why we have to draw the proverbial line in the sand at some point. |
Sorry, I was unclear — 6.18 does have it. Stable backporting worked exactly as you'd expect. 82163d6 landed in v6.13 and is an ancestor of v6.18 ( The catch is that it's two different call sites. On this branch:
Upstream couldn't have fixed the second one, because upstream doesn't have that function. It was added here after the fix landed, and the same defect came with it. So this isn't a gap in stable; it's a fix that exists on one path and not its sibling. Opened #348 with just those two bug fixes, based on this branch at 580caa3. 10 insertions, 5 deletions, drivetemp.c only.
Understood, and I'll drop the retier — your call on where the line sits, and I'm not going to argue scope on hardware you don't qualify. One thing worth recording for whoever hits this next, though, since it isn't specific to the NetApp drives: Reference Temperature is defined That's a note, not a request. I'll handle the false alerts on the middleware side instead, where it doesn't touch a fleet-wide ABI, and close #347 once #348 is dealt with. |
2 participants
Jira: https://ixsystems.atlassian.net/browse/NAS-142826
Healthy SAS drives raise a permanent
DiskTemperatureTooHotCRITICAL alert because the SCSI path indrivers/hwmon/drivetemp.cmaps log page 0Dh parameter 0001h (REFERENCE TEMPERATURE, a continuous-operation ceiling with ETC = 0) totemp1_crit. It belongs intemp1_max, as the SATA path in the same driver, nvme and k10temp already do. Two latent bugs in the same path are fixed first.Four commits, in order:
blk_rq_map_kern()bounces it. Usest->smartdatalike the ATA path; bound the temperature byte read.scsi_execute_cmd()returns the positivescmd->resulton CHECK CONDITION;hwmon_attr_show()only rejects negatives, so an uninitialised stacklongreaches userspace as the temperature. Normalise to-EIOlike the SATA path.temp1_critat all, so the middleware side (truenas/middleware PR linked on the Jira) must land in the same release; the existing alert source skips disks with no crit.Fixes:points at 06ff843, the SCSI-support commit on this branch. The same series applies cleanly totruenas/linux-6.12withFixes: 43ff910ef5cc.Verification: applies cleanly to
truenas/linux-6.18at 580caa3;checkpatch.plis silent on 1–3, and 4 emits only messages from matching the file's existing style (return (x);, 4-space prototype continuation). Not yet compile-tested or run on hardware — the item that most needs a real SPC-5 drive is the subpage 02h byte offsets in commit 4 (trigger vs reset fields;sg_logs -p 0x0d,0x2 -H).