RE: [PATCH] rtc: snvs: fix possible race condition

[Anson Huang]

Hi, Trent

> On Thu, 2019-07-18 at 03:08 +0000, Aisheng Dong wrote:
> > > From: Anson Huang
> > > Sent: Wednesday, July 17, 2019 9:58 PM> Hi, Aisheng
> > >
> > > > > From: Anson.Huang@xxxxxxx <Anson.Huang@xxxxxxx>
> > > > > Sent: Tuesday, July 16, 2019 3:19 PM
> > > > >
> > > > > The RTC IRQ is requested before the struct rtc_device is
> > > > > allocated, this may lead to a NULL pointer dereference in IRQ
> > > > > handler.
> > > > >
> > > > > To fix this issue, allocating the rtc_device struct before
> > > > > requesting the RTC IRQ using devm_rtc_allocate_device, and use
> > > > > rtc_register_device to register the RTC device.
> > > > >
> > > >
> > > > I saw other rtc drivers did the same way as us, so this looks like
> > > > a common problem.
> > > > My question is if we can clear interrupt status before register to
> > > > avoid this issue as other rtc drivers?
> > >
> > > I think we can NOT predict when the IRQ will be pending, IRQ could
> > > arrive at any time, the most safe way is to prepare everything
> > > before requesting/enabling IRQ.
> > > There is also patch to fix similar issue:
> 
> I think one could attempt to disable all irq sources in the device via its
> register space, then enable the interrupt.  But this seems more specific to
> each device than changing the pattern of device registration, so IMHO, it's
> not really better.
> 
> I do worry that handling the irq before the rtc device is registered could still
> result in a crash.  From what I saw, the irq path in snvs only uses driver state
> members that are fully initialized for the most part, and the allocated but
> unregistered data->rtc is only used in one call to rtc_update_irq(), which
> appears to be ok with this.
> 
> But it is not that hard to imagine that something could go into the rtc core
> that assumes call like rtc_update_irq() are only made on registered devices.
> 
> If there was a way to do it, I think allocating the irq in a masked state and
> then unmasking it as part of the final registration call to make the device go
> live would be a safer and more general pattern.

It makes sense, I think we can just move the devm_request_irq() to after rtc_register_device(),
It will make sure everything is ready before IRQ is enabled. Will send out a V2 patch. 

Thanks,
Anson