linux-rtc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rtc: snvs: fix possible race condition
@ 2019-07-16  7:18 Anson.Huang
  2019-07-17 10:54 ` Aisheng Dong
  2019-08-29 15:39 ` Alexandre Belloni
  0 siblings, 2 replies; 10+ messages in thread
From: Anson.Huang @ 2019-07-16  7:18 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni, linux-rtc, linux-kernel; +Cc: Linux-imx

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(-)

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* RE: [PATCH] rtc: snvs: fix possible race condition
  2019-07-16  7:18 [PATCH] rtc: snvs: fix possible race condition Anson.Huang
@ 2019-07-17 10:54 ` Aisheng Dong
  2019-07-17 13:57   ` Anson Huang
  2019-08-29 15:39 ` Alexandre Belloni
  1 sibling, 1 reply; 10+ messages in thread
From: Aisheng Dong @ 2019-07-17 10:54 UTC (permalink / raw)
  To: Anson Huang, a.zummo, alexandre.belloni, linux-rtc, linux-kernel
  Cc: dl-linux-imx

> 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] rtc: snvs: fix possible race condition
  2019-07-17 10:54 ` Aisheng Dong
@ 2019-07-17 13:57   ` Anson Huang
  2019-07-18  3:08     ` Aisheng Dong
  0 siblings, 1 reply; 10+ messages in thread
From: Anson Huang @ 2019-07-17 13:57 UTC (permalink / raw)
  To: Aisheng Dong, a.zummo, alexandre.belloni, linux-rtc, linux-kernel
  Cc: dl-linux-imx

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 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] rtc: snvs: fix possible race condition
  2019-07-17 13:57   ` Anson Huang
@ 2019-07-18  3:08     ` Aisheng Dong
  2019-07-18 16:32       ` Trent Piepho
  0 siblings, 1 reply; 10+ messages in thread
From: Aisheng Dong @ 2019-07-18  3:08 UTC (permalink / raw)
  To: Anson Huang, a.zummo, alexandre.belloni, linux-rtc, linux-kernel
  Cc: dl-linux-imx

> 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] rtc: snvs: fix possible race condition
  2019-07-18  3:08     ` Aisheng Dong
@ 2019-07-18 16:32       ` Trent Piepho
  2019-07-19  2:57         ` Anson Huang
  0 siblings, 1 reply; 10+ messages in thread
From: Trent Piepho @ 2019-07-18 16:32 UTC (permalink / raw)
  To: linux-rtc, anson.huang, a.zummo, linux-kernel, alexandre.belloni,
	aisheng.dong
  Cc: linux-imx

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.

> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] rtc: snvs: fix possible race condition
  2019-07-18 16:32       ` Trent Piepho
@ 2019-07-19  2:57         ` Anson Huang
  2019-07-19 19:04           ` Trent Piepho
  0 siblings, 1 reply; 10+ messages in thread
From: Anson Huang @ 2019-07-19  2:57 UTC (permalink / raw)
  To: Trent Piepho, linux-rtc, a.zummo, linux-kernel,
	alexandre.belloni, Aisheng Dong
  Cc: dl-linux-imx

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] rtc: snvs: fix possible race condition
  2019-07-19  2:57         ` Anson Huang
@ 2019-07-19 19:04           ` Trent Piepho
  2019-07-20 19:55             ` Alexandre Belloni
  0 siblings, 1 reply; 10+ messages in thread
From: Trent Piepho @ 2019-07-19 19:04 UTC (permalink / raw)
  To: linux-rtc, anson.huang, a.zummo, linux-kernel, alexandre.belloni,
	aisheng.dong
  Cc: linux-imx

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?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] rtc: snvs: fix possible race condition
  2019-07-19 19:04           ` Trent Piepho
@ 2019-07-20 19:55             ` Alexandre Belloni
  2019-08-13  9:22               ` Anson Huang
  0 siblings, 1 reply; 10+ messages in thread
From: Alexandre Belloni @ 2019-07-20 19:55 UTC (permalink / raw)
  To: Trent Piepho
  Cc: linux-rtc, anson.huang, a.zummo, linux-kernel, aisheng.dong, linux-imx

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.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] rtc: snvs: fix possible race condition
  2019-07-20 19:55             ` Alexandre Belloni
@ 2019-08-13  9:22               ` Anson Huang
  0 siblings, 0 replies; 10+ messages in thread
From: Anson Huang @ 2019-08-13  9:22 UTC (permalink / raw)
  To: Alexandre Belloni, Trent Piepho
  Cc: linux-rtc, a.zummo, linux-kernel, Aisheng Dong, dl-linux-imx

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] rtc: snvs: fix possible race condition
  2019-07-16  7:18 [PATCH] rtc: snvs: fix possible race condition Anson.Huang
  2019-07-17 10:54 ` Aisheng Dong
@ 2019-08-29 15:39 ` Alexandre Belloni
  1 sibling, 0 replies; 10+ messages in thread
From: Alexandre Belloni @ 2019-08-29 15:39 UTC (permalink / raw)
  To: Anson.Huang; +Cc: a.zummo, linux-rtc, linux-kernel, Linux-imx

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.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-08-29 15:39 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-16  7:18 [PATCH] rtc: snvs: fix possible race condition Anson.Huang
2019-07-17 10:54 ` Aisheng Dong
2019-07-17 13:57   ` Anson Huang
2019-07-18  3:08     ` Aisheng Dong
2019-07-18 16:32       ` Trent Piepho
2019-07-19  2:57         ` Anson Huang
2019-07-19 19:04           ` Trent Piepho
2019-07-20 19:55             ` Alexandre Belloni
2019-08-13  9:22               ` Anson Huang
2019-08-29 15:39 ` Alexandre Belloni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).