| Message ID | 20190716071858.36750-1-Anson.Huang@nxp.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | rtc: snvs: fix possible race condition | expand |
> 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? Regards Aisheng > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > drivers/rtc/rtc-snvs.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c index > 7ee673a2..4f9a107 100644 > --- a/drivers/rtc/rtc-snvs.c > +++ b/drivers/rtc/rtc-snvs.c > @@ -279,6 +279,10 @@ static int snvs_rtc_probe(struct platform_device > *pdev) > if (!data) > return -ENOMEM; > > + data->rtc = devm_rtc_allocate_device(&pdev->dev); > + if (IS_ERR(data->rtc)) > + return PTR_ERR(data->rtc); > + > data->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, > "regmap"); > > if (IS_ERR(data->regmap)) { > @@ -343,10 +347,9 @@ static int snvs_rtc_probe(struct platform_device > *pdev) > goto error_rtc_device_register; > } > > - data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name, > - &snvs_rtc_ops, THIS_MODULE); > - if (IS_ERR(data->rtc)) { > - ret = PTR_ERR(data->rtc); > + data->rtc->ops = &snvs_rtc_ops; > + ret = rtc_register_device(data->rtc); > + if (ret) { > dev_err(&pdev->dev, "failed to register rtc: %d\n", ret); > goto error_rtc_device_register; > } > -- > 2.7.4
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: commit 060711f5274dfc2d76a5b2cd65abf6ccbf061e40 Author: Alexandre Belloni <alexandre.belloni@bootlin.com> Date: Tue Apr 30 11:32:09 2019 +0200 rtc: digicolor: fix possible race condition Anson
> 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 just feel like it's common issue. But seems community already did the same thing. So: Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com> Regards Aisheng > commit 060711f5274dfc2d76a5b2cd65abf6ccbf061e40 > Author: Alexandre Belloni <alexandre.belloni@bootlin.com> > Date: Tue Apr 30 11:32:09 2019 +0200 > > rtc: digicolor: fix possible race condition > > Anson
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. >
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@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. 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
On Fri, 2019-07-19 at 02:57 +0000, Anson Huang wrote: > > > 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. That will mean registering the rtc, then unregistering it if the irq request fails. More of a pain to write this failure path. Alexandre, is it part of rtc core design that rtc_update_irq() might be called on a rtc device that is properly allocated, but not registered yet?
On 19/07/2019 19:04:21+0000, Trent Piepho wrote: > On Fri, 2019-07-19 at 02:57 +0000, Anson Huang wrote: > > > > > 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. > > That will mean registering the rtc, then unregistering it if the irq > request fails. More of a pain to write this failure path. > > Alexandre, is it part of rtc core design that rtc_update_irq() might be > called on a rtc device that is properly allocated, but not registered > yet? Yes, the main reason of the change of API was exactly to handle this.
Hi, Alexandre > On 19/07/2019 19:04:21+0000, Trent Piepho wrote: > > On Fri, 2019-07-19 at 02:57 +0000, Anson Huang wrote: > > > > > > > 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. > > > > That will mean registering the rtc, then unregistering it if the irq > > request fails. More of a pain to write this failure path. > > > > Alexandre, is it part of rtc core design that rtc_update_irq() might > > be called on a rtc device that is properly allocated, but not > > registered yet? > > Yes, the main reason of the change of API was exactly to handle this. What about this patch's status? Should we continue or any change needed? https://patchwork.ozlabs.org/patch/1132481/ Thanks, Anson
On 16/07/2019 15:18:58+0800, Anson.Huang@nxp.com wrote: > From: Anson Huang <Anson.Huang@nxp.com> > > 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. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > drivers/rtc/rtc-snvs.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > Applied, thanks.
diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c index 7ee673a2..4f9a107 100644 --- a/drivers/rtc/rtc-snvs.c +++ b/drivers/rtc/rtc-snvs.c @@ -279,6 +279,10 @@ static int snvs_rtc_probe(struct platform_device *pdev) if (!data) return -ENOMEM; + data->rtc = devm_rtc_allocate_device(&pdev->dev); + if (IS_ERR(data->rtc)) + return PTR_ERR(data->rtc); + data->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "regmap"); if (IS_ERR(data->regmap)) { @@ -343,10 +347,9 @@ static int snvs_rtc_probe(struct platform_device *pdev) goto error_rtc_device_register; } - data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name, - &snvs_rtc_ops, THIS_MODULE); - if (IS_ERR(data->rtc)) { - ret = PTR_ERR(data->rtc); + data->rtc->ops = &snvs_rtc_ops; + ret = rtc_register_device(data->rtc); + if (ret) { dev_err(&pdev->dev, "failed to register rtc: %d\n", ret); goto error_rtc_device_register; }