Re: [PATCH 1/2] hwmon: (lm63) Convert macro to function to avoid TOCTOU

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Incomplete TOCTOU fix allows compiler-induced divide-by-zero crash in `show_fan()`.
- [Medium] Lockless sysfs read in `show_fan()` causes torn reads and violates hwmon subsystem locking guidelines.
--

commit ebc10731137639981842ea19f80f0b969db71842
Author: Gui-Dong Han <hanguidong02@gmail.com>

hwmon: (lm63) Convert macro to function to avoid TOCTOU

The patch converts the FAN_FROM_REG macro to a static function
fan_from_reg() to evaluate its argument only once, avoiding a TOCTOU
race and potential divide-by-zero error.

Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/

> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 035176a98ce9c..da132b267c584 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
> @@ -109,8 +109,14 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
>   * adapted accordingly.
>   */
>  
> -#define FAN_FROM_REG(reg)	((reg) == 0xFFFC || (reg) == 0 ? 0 : \
> -				 5400000 / (reg))
> +static int fan_from_reg(int reg)
> +{
> +	if (reg == 0xFFFC || reg == 0)
> +		return 0;
> +
> +	return 5400000 / reg;
> +}
> +

[ ... ]

> @@ -333,7 +339,7 @@ static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>  	struct lm63_data *data = lm63_update_device(dev);
> -	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index]));
> +	return sprintf(buf, "%d\n", fan_from_reg(data->fan[attr->index]));

Since fan_from_reg() is a small static function, won't the compiler likely
inline it?

If it gets inlined, couldn't the compiler optimize the code by reloading
data->fan[attr->index] from memory multiple times (once for the 0 check, and
again for the division)?

If lm63_update_device() concurrently writes to data->fan[0] in two non-atomic
steps (where the first step can momentarily set the value to 0), and a reload
happens exactly when the value is 0, could this still cause a divide-by-zero
panic?

Note that the subsequent patch in the series ('hwmon: (lm63) Add locking
to avoid TOCTOU') explicitly skips show_fan(), leaving this unfixed.

Additionally, is it safe to read data->fan locklessly here?

According to the hwmon subsystem guidelines, drivers must implement locking
for attributes registered outside the core-serialized info API. Because this
driver registers its attributes using devm_hwmon_device_register_with_groups(),
it should lock its attribute accesses.

Since lm63_update_device() releases the update_lock before returning, and
data->fan[0] is updated non-atomically in two steps, couldn't a concurrent
sysfs read observe a partially updated 16-bit state?

Could this torn read result in wildly inaccurate fan speed values being
reported? The subsequent patch ('hwmon: (lm63) Add locking to avoid TOCTOU')
fixes similar locking issues but explicitly omits show_fan().

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260416090752.97392-1-hanguidong02@gmail.com?part=1