On 9/22/22 13:37, Li Zhong wrote:
On Wed, Sep 21, 2022 at 8:16 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
On 9/21/22 16:31, Li Zhong wrote:
Hello,
My static analysis tool reports a possible bug in the adm9240 driver in Linux
v6.0:
drivers/hwmon/adm9240.c:
adm9240_read()
adm9240_fan_read() --> Line 509
adm9240_write_fan_div()
adm9240_write_fan_div() says 'callers must hold
data->update_lock'. However, it seems like the context does
not hold the lock it requires. So it may cause data race when
setting new fan div.
I am not quite sure whether this is a real bug. Any feedback would be
appreciated!
You are correct, the code in adm9240_fan_read() should acquire
the mutex before calling adm9240_write_fan_div() and while
manipulating data->fan_div[channel].
Guenter
Thanks for your patient reply! Can I submit a patch on this? The draft will
be something like:
+ mutex_lock(&data->update_lock)
err = adm9240_write_fan_div(data, channel, ++data->fan_div[channel]);
if (err)
return err;
+ mutex_unlock(&data->update_lock);
Let me know if you have any suggestions!
That would leave the mutex in locked state after an error, and it does not
take into account that data->fan_div[channel] might still change after being
checked but before being used. The lock has to be around the if() statement,
and the lock must be released after an error was observed.
At the very least, the code has to be something like
...
mutex_lock(&data->update_lock);
if (regval == 255 && data->fan_div[channel] < 3) {
/* adjust fan clock divider on overflow */
err = adm9240_write_fan_div(data, channel,
++data->fan_div[channel]);
if (err) {
mutex_unlock(&data->update_lock);
return err;
}
}
mutex_unlock(&data->update_lock);
...
However, that isn't perfect since the fan divisor and the fan speed
register value are not in sync. Technically it needs to be something like
u8 fan_div;
...
mutex_lock(&data->update_lock);
err = regmap_read(data->regmap, ADM9240_REG_FAN(channel), ®val);
if (err) {
mutex_unlock(&data->update_lock);
return err;
}
fan_div = data->fan_div[channel];
if (regval == 255 && fan_div < 3) {
err = adm9240_write_fan_div(data, channel, fan_div + 1);
if (err) {
mutex_unlock(&data->update_lock);
return err;
}
data->fan_div[channel] = fan_div + 1;
}
mutex_unlock(&data->update_lock);
*val = FAN_FROM_REG(regval, BIT(fan_div));
break;
Thanks,
Guenter
