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

From: Trent Piepho <tpiepho@impinj.com>
To: "linux-rtc@vger.kernel.org" <linux-rtc@vger.kernel.org>,
	"anson.huang@nxp.com" <anson.huang@nxp.com>,
	"a.zummo@towertech.it" <a.zummo@towertech.it>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"alexandre.belloni@bootlin.com" <alexandre.belloni@bootlin.com>,
	"aisheng.dong@nxp.com" <aisheng.dong@nxp.com>
Cc: "linux-imx@nxp.com" <linux-imx@nxp.com>
Subject: Re: [PATCH] rtc: snvs: fix possible race condition
Date: Thu, 18 Jul 2019 16:32:06 +0000	[thread overview]
Message-ID: <1563467526.2343.80.camel@impinj.com> (raw)
In-Reply-To: <AM0PR04MB421114F025F27AF2BC5FA21980C80@AM0PR04MB4211.eurprd04.prod.outlook.com>

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@nxp.com <Anson.Huang@nxp.com>
> > > > 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.

> 