Commit Message

Hello,
you're receiving this email because you were involved in the
development of one or more RTC drivers.
Recently it has been brought to my attention that there's
a race condition in some drivers [1].
If your drivers bails out after the rtc_device_register() call,
it may trigger the condition.
There are two things that drivers often do after rtc_device_register():
a) register sysfs entries
b) request irqs
It seems that most drivers could still work correctly
if those do not succeed.
The proposed course of action is to modify the drivers in
order to avoid bailing out (and notify the user where appropriate).
Any other action that might trigger a bail out should be done
before rtc_device_register.
I've modified 22 of them in order to provide an example
of what the fix might look like (patch attached).
Julia Lawall has been kind enough to write a semantic file
for coccinelle that quickly found the affected files [2].
I'd appreciate if you can give a look to your driver and
provide a patch if appropriate. If you're lucky I've already patched it
and you just have to check if my work makes sense to you.
Thanks for your cooperation.
[1] https://lkml.org/lkml/2014/2/26/185
[2]
drivers/rtc/rtc-ab8500.c
drivers/rtc/rtc-as3722.c
drivers/rtc/rtc-at91sam9.c
drivers/rtc/rtc-bfin.c
drivers/rtc/rtc-cmos.c
drivers/rtc/rtc-da9055.c
drivers/rtc/rtc-davinci.c
drivers/rtc/rtc-ds1305.c
drivers/rtc/rtc-ds1307.c
drivers/rtc/rtc-ds1511.c
drivers/rtc/rtc-ds1553.c
drivers/rtc/rtc-ds1672.c
drivers/rtc/rtc-ds1742.c
drivers/rtc/rtc-ds3232.c
drivers/rtc/rtc-ep93xx.c
drivers/rtc/rtc-isl1208.c
drivers/rtc/rtc-jz4740.c
drivers/rtc/rtc-m41t80.c
drivers/rtc/rtc-m48t59.c
drivers/rtc/rtc-max77686.c
drivers/rtc/rtc-max8907.c
drivers/rtc/rtc-max8997.c
drivers/rtc/rtc-mc13xxx.c
drivers/rtc/rtc-mrst.c
drivers/rtc/rtc-nuc900.c
drivers/rtc/rtc-omap.c
drivers/rtc/rtc-palmas.c
drivers/rtc/rtc-pcap.c
drivers/rtc/rtc-pcf2123.c
drivers/rtc/rtc-pl031.c
drivers/rtc/rtc-pm8xxx.c
drivers/rtc/rtc-rp5c01.c
drivers/rtc/rtc-rs5c372.c
drivers/rtc/rtc-rv3029c2.c
drivers/rtc/rtc-rx8025.c
drivers/rtc/rtc-s3c.c
drivers/rtc/rtc-s5m.c
drivers/rtc/rtc-sirfsoc.c
drivers/rtc/rtc-stk17ta8.c
drivers/rtc/rtc-stmp3xxx.c
drivers/rtc/rtc-tegra.c
drivers/rtc/rtc-test.c
drivers/rtc/rtc-tps6586x.c
drivers/rtc/rtc-tps80031.c
drivers/rtc/rtc-twl.c
drivers/rtc/rtc-tx4939.c
drivers/rtc/rtc-vr41xx.c
drivers/rtc/rtc-vt8500.c
drivers/rtc/rtc-x1205.c

Comments

Суббота, 1 марта 2014, 0:37 +01:00 от Alessandro Zummo <a.zummo@towertech.it>:
> > Hello,> > you're receiving this email because you were involved in the> development of one or more RTC drivers.> > Recently it has been brought to my attention that there's> a race condition in some drivers [1].> > If your drivers bails out after the rtc_device_register() call,> it may trigger the condition.> > There are two things that drivers often do after rtc_device_register():> > a) register sysfs entries> b) request irqs> > It seems that most drivers could still work correctly> if those do not succeed.> > The proposed course of action is to modify the drivers in> order to avoid bailing out (and notify the user where appropriate).> > Any other action that might trigger a bail out should be done> before rtc_device_register.> > I've modified 22 of them in order to provide an example> of what the fix might look like (patch attached).> > Julia Lawall has been kind enough to write a semantic file> for coccinelle that quickly found the affected files [2].> > I'd appreciate if you can give a look to your driver and> provide a patch if appropriate. If you're lucky I've already patched it> and you just have to check if my work makes sense to you.> > Thanks for your cooperation.
The patch for rtc-mc13xxx looks incorrect.
Interrupts are released regardless of the result of devm_rtc_device_register().
In any case linux-next branch already contains a slightly modified version of the driver.
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/rtc/rtc-mc13xxx.c?id=06ad9825e7048f4385b77784286bf9595439e1bc
---

On Sat, 01 Mar 2014 09:14:45 +0400
Alexander Shiyan <shc_work@mail.ru> wrote:
> The patch for rtc-mc13xxx looks incorrect.> Interrupts are released regardless of the result of devm_rtc_device_register().
Will drop it, thanks. Got confused by those goto labels within an if.
> In any case linux-next branch already contains a slightly modified version of the driver.> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/rtc/rtc-mc13xxx.c?id=06ad9825e7048f4385b77784286bf9595439e1bc
Can you make sure this new version behaves correctly wrto
the race condition??

Суббота, 1 марта 2014, 12:28 +01:00 от Alessandro Zummo <a.zummo@towertech.it>:
> On Sat, 01 Mar 2014 09:14:45 +0400> Alexander Shiyan <shc_work@mail.ru> wrote:> > > The patch for rtc-mc13xxx looks incorrect.> > Interrupts are released regardless of the result of devm_rtc_device_register().> > Will drop it, thanks. Got confused by those goto labels within an if.> > > In any case linux-next branch already contains a slightly modified version of the driver.> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/rtc/rtc-mc13xxx.c?id=06ad9825e7048f4385b77784286bf9595439e1bc> > Can you make sure this new version behaves correctly wrto> the race condition??
I am sure that I do not fully understand the race condition problem.
From my perspective, everything looks correct.
---

On Sat, 01 Mar 2014 21:52:34 +0900 (JST)
Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> Please do not do this cleanup. With this cleanup, pdata->rtc will be> an errno value instead of NULL just after devm_rtc_device_register> error. This breaks "if (likely(pdata->rtc))" checking in interrupt> handler.
Ok. Will the handler be called even if the rtc device is not registered?

On Sat, 01 Mar 2014 18:07:51 +0400
Alexander Shiyan <shc_work@mail.ru> wrote:
> > Can you make sure this new version behaves correctly wrto> > the race condition?> > I am sure that I do not fully understand the race condition problem.> From my perspective, everything looks correct.
Basically your driver should NOT fail after
priv->rtc = devm_rtc_device_register()
if you can work without IRQs, just return 0 at the end of the probe
function

On Sat, 1 Mar 2014 15:10:52 +0100, Alessandro Zummo <a.zummo@towertech.it> wrote:
> Ok. Will the handler be called even if the rtc device is not registered?
I guess so. Since request_irq is called before rtc_device_register,
the handler might be called by alarm or something or interrupt from
other devices shareing the irq line.

Суббота, 1 марта 2014, 15:27 +01:00 от Alessandro Zummo <a.zummo@towertech.it>:
> On Sat, 01 Mar 2014 18:07:51 +0400> Alexander Shiyan <shc_work@mail.ru> wrote:> > > > Can you make sure this new version behaves correctly wrto> > > the race condition?> > > > I am sure that I do not fully understand the race condition problem.> > From my perspective, everything looks correct.> > Basically your driver should NOT fail after> > priv->rtc = devm_rtc_device_register()> > if you can work without IRQs, just return 0 at the end of the probe> function
Yeah, okay. In this case, the driver can operate without interrupts,
we only need to make dynamic assignment of alarm functions (read_alarm(),
set_alarm() and alarm_irq_enable()) if mc13xxx_irq_request_nounmask()
for the alarm IRQ success.
If it's not too important, I'll make a patch a bit later.
---

On Sat, 01 Mar 2014 18:40:41 +0400
Alexander Shiyan <shc_work@mail.ru> wrote:
> > if you can work without IRQs, just return 0 at the end of the probe> > function > > Yeah, okay. In this case, the driver can operate without interrupts,> we only need to make dynamic assignment of alarm functions (read_alarm(),> set_alarm() and alarm_irq_enable()) if mc13xxx_irq_request_nounmask()> for the alarm IRQ success.> If it's not too important, I'll make a patch a bit later.
or you can request the irqs before the rtc_device_register call.
no hurry, just drop me a note when you're done so I can keep track
of the while process.
thanks!

On Sat, 01 Mar 2014 23:36:57 +0900 (JST)
Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> I guess so. Since request_irq is called before rtc_device_register,> the handler might be called by alarm or something or interrupt from> other devices shareing the irq line.
Ok, then it would be better to move the request_irq after the registration
call. I have no access to systems with the two rtc you can working
on, any chance you can make such a mod?

Суббота, 1 марта 2014, 15:45 +01:00 от Alessandro Zummo <a.zummo@towertech.it>:
> On Sat, 01 Mar 2014 18:40:41 +0400> Alexander Shiyan <shc_work@mail.ru> wrote:> > > > if you can work without IRQs, just return 0 at the end of the probe> > > function > > > > Yeah, okay. In this case, the driver can operate without interrupts,> > we only need to make dynamic assignment of alarm functions (read_alarm(),> > set_alarm() and alarm_irq_enable()) if mc13xxx_irq_request_nounmask()> > for the alarm IRQ success.> > If it's not too important, I'll make a patch a bit later.> > or you can request the irqs before the rtc_device_register call.
This was the reason why I moved IRQ initialization interrupt after RTC registration.
This can be redone back if we have the ability to add !IS_ERR_OR_NULL(rtc)
check in the begin of rtc_update_irq().
> no hurry, just drop me a note when you're done so I can keep track> of the while process.
OK.
---

On Sat, 01 Mar 2014 18:56:29 +0400
Alexander Shiyan <shc_work@mail.ru> wrote:
> This was the reason why I moved IRQ initialization interrupt after RTC registration.> This can be redone back if we have the ability to add !IS_ERR_OR_NULL(rtc)> check in the begin of rtc_update_irq().
good catch, ty.
void rtc_update_irq(struct rtc_device *rtc,
unsigned long num, unsigned long events)
{
if (unlikely(IS_ERR_OR_NULL(rtc)))
return;
pm_stay_awake(rtc->dev.parent);
schedule_work(&rtc->irqwork);
}

Суббота, 1 марта 2014, 16:05 +01:00 от Alessandro Zummo <a.zummo@towertech.it>:
> On Sat, 01 Mar 2014 18:56:29 +0400> Alexander Shiyan <shc_work@mail.ru> wrote:> > > This was the reason why I moved IRQ initialization interrupt after RTC registration.> > This can be redone back if we have the ability to add !IS_ERR_OR_NULL(rtc)> > check in the begin of rtc_update_irq().> > good catch, ty.> > void rtc_update_irq(struct rtc_device *rtc,> unsigned long num, unsigned long events)> {> if (unlikely(IS_ERR_OR_NULL(rtc)))> return;> > pm_stay_awake(rtc->dev.parent);> schedule_work(&rtc->irqwork);> }
Yes, that's very good.
Can You send this into mainline and make signed tag?
It can also eliminate such bugs in other drivers.
---

On Sat, 1 Mar 2014 15:51:53 +0100, Alessandro Zummo <a.zummo@towertech.it> wrote:
> Ok, then it would be better to move the request_irq after the registration> call. I have no access to systems with the two rtc you can working> on, any chance you can make such a mod?
I'm bit uncomfortable if rtc_device_register() was called before all
setup (including request_irq) have completed.
Now your "rtc: verify a critical argument to rtc_update_irq() before
using it" patch is available, I'm happy with your first example patch.
---
Atsushi Nemoto

On Fri, Feb 28, 2014 at 6:37 PM, Alessandro Zummo wrote:
> you're receiving this email because you were involved in the> development of one or more RTC drivers.>> Recently it has been brought to my attention that there's> a race condition in some drivers [1].>> If your drivers bails out after the rtc_device_register() call,> it may trigger the condition.>> There are two things that drivers often do after rtc_device_register():>> a) register sysfs entries> b) request irqs>> It seems that most drivers could still work correctly> if those do not succeed.
the Blackfin RTC can't provide alarms and such sanely w/out the IRQ.
it could provide set/get of the time, but i'm not sure that's even
desirable. if something else has requested the RTC IRQ, it kind of
means something else is using that hardware, so it kind of should
bail. but the likelyhood of that is probably remote to the point
where it can be ignored.
> The proposed course of action is to modify the drivers in> order to avoid bailing out (and notify the user where appropriate).>> Any other action that might trigger a bail out should be done> before rtc_device_register.>> I've modified 22 of them in order to provide an example> of what the fix might look like (patch attached).
full of dos ^M gremlins ;)
-mike