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

On Thu, Apr 16, 2026 at 6:41 PM <[email protected]> wrote:
>
> 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 <[email protected]>
>
> 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)?

I am not an expert on compiler optimization. I consulted a few
different models and got mixed answers. Unlike a macro with a side
effect (like ++), an inlined function doesn't strictly force multiple
memory fetches, so it's a bit ambiguous.

>
> 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?

Your point about data->fan[0] being updated non-atomically in two
steps is a very nice finding. This gives me a solid reason to just add
locking to show_fan() and fix the torn read properly.

>
> 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/[email protected]?part=1