From: Li Zhong <floridsleeves@gmail.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org, jdelvare@suse.com
Subject: Re: [BUG] drivers: adm9240: possible data race bug in adm9240_fan_read()
Date: Thu, 22 Sep 2022 16:46:52 -0700 [thread overview]
Message-ID: <CAMEuxRo+3UgPCX44OGWZBvhOC6V+ocLYwjOLjYW7UHF=5KBrxQ@mail.gmail.com> (raw)
In-Reply-To: <1420a755-6f8c-e1e2-2a34-dcb535730f7a@roeck-us.net>
On Thu, Sep 22, 2022 at 2:53 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 9/22/22 13:37, Li Zhong wrote:
> > On Wed, Sep 21, 2022 at 8:16 PM Guenter Roeck <linux@roeck-us.net> 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.
prev parent reply other threads:[~2022-09-22 23:47 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-21 23:31 [BUG] drivers: adm9240: possible data race bug in adm9240_fan_read() Li Zhong
2022-09-22 3:16 ` Guenter Roeck
2022-09-22 20:37 ` Li Zhong
2022-09-22 21:53 ` Guenter Roeck
2022-09-22 23:46 ` Li Zhong [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMEuxRo+3UgPCX44OGWZBvhOC6V+ocLYwjOLjYW7UHF=5KBrxQ@mail.gmail.com' \
--to=floridsleeves@gmail.com \
--cc=jdelvare@suse.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux@roeck-us.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.