On Thu, Sep 22, 2022 at 2:53 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> 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
Thanks for your suggestions and drafts! I submit the patch.
