[PATCH] rtc: st-lpc: remove unnecessary check

[PATCH] rtc: st-lpc: remove unnecessary check

[Alexandre Belloni]

The RTC core already ensures the alarm is set to a time in the future, it
is not necessary to check again in the driver.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-st-lpc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/rtc/rtc-st-lpc.c b/drivers/rtc/rtc-st-lpc.c
index bee75ca7ff79..ce2dae6e2a24 100644
--- a/drivers/rtc/rtc-st-lpc.c
+++ b/drivers/rtc/rtc-st-lpc.c
@@ -166,10 +166,6 @@ static int st_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	now_secs = rtc_tm_to_time64(&now);
 	alarm_secs = rtc_tm_to_time64(&t->time);
 
-	/* Invalid alarm time */
-	if (now_secs > alarm_secs)
-		return -EINVAL;
-
 	memcpy(&rtc->alarm, t, sizeof(struct rtc_wkalrm));
 
 	/* Now many secs to fire */
-- 
2.20.1

Re: [PATCH] rtc: st-lpc: remove unnecessary check

[Trent Piepho]

On Tue, 2019-04-30 at 22:18 +0200, Alexandre Belloni wrote:
> The RTC core already ensures the alarm is set to a time in the future, it
> is not necessary to check again in the driver.

My reading of the rtc core code is that it checks if the alarm is in
the future *twice* before handing off the set call to the driver, which
possibly checks a 3rd time (as seen here).

However, all these checks are done *before* setting the alarm.  It
still possible to have a race and set the alarm after the time has
already passed, in which case the alarm will never fire.

The way to fix the race would be to have the driver check the alarm
*after* setting it.  In precisely this order, do these steps:

1. Set alarm in RTC, to Talarm
2. Get time from RTC, as Tcurrent
3. Get alarm status from RTC

If Talarm < Tcurrent, alarm was set to future time, no error
Else
  If status == fired, alarm was set and has since fired, no error
  Else status == not fired, alarm was set in past, EINVAL

This should be race free.


>  
> -	/* Invalid alarm time */
> -	if (now_secs > alarm_secs)
> -		return -EINVAL;
> -
>  	memcpy(&rtc->alarm, t, sizeof(struct rtc_wkalrm));
>  
>  	/* Now many secs to fire */
        alarm_secs -= now_secs;
        lpa = (unsigned long long)alarm_secs * rtc->clkrate;

While it's true the time wouldn't normally be in past, it still races,
as describe above. In that case, the math here underflows alarm_secs,
so it probably still makes sense to check.

Re: [PATCH] rtc: st-lpc: remove unnecessary check

[Alexandre Belloni]

On 30/04/2019 22:31:19+0000, Trent Piepho wrote:
> On Tue, 2019-04-30 at 22:18 +0200, Alexandre Belloni wrote:
> > The RTC core already ensures the alarm is set to a time in the future, it
> > is not necessary to check again in the driver.
> 
> My reading of the rtc core code is that it checks if the alarm is in
> the future *twice* before handing off the set call to the driver, which
> possibly checks a 3rd time (as seen here).
> 
> However, all these checks are done *before* setting the alarm.  It
> still possible to have a race and set the alarm after the time has
> already passed, in which case the alarm will never fire.
> 

I agree the core need to handle that possible race better and this is
something I'm planning to work on.

> The way to fix the race would be to have the driver check the alarm
> *after* setting it.  In precisely this order, do these steps:
> 
> 1. Set alarm in RTC, to Talarm
> 2. Get time from RTC, as Tcurrent
> 3. Get alarm status from RTC
> 
> If Talarm < Tcurrent, alarm was set to future time, no error

This should be Talarm > Tcurrent, right?

> Else
>   If status == fired, alarm was set and has since fired, no error
>   Else status == not fired, alarm was set in past, EINVAL
> 
> This should be race free.
> 
> 
> >  
> > -	/* Invalid alarm time */
> > -	if (now_secs > alarm_secs)
> > -		return -EINVAL;
> > -
> >  	memcpy(&rtc->alarm, t, sizeof(struct rtc_wkalrm));
> >  
> >  	/* Now many secs to fire */
>         alarm_secs -= now_secs;
>         lpa = (unsigned long long)alarm_secs * rtc->clkrate;
> 
> While it's true the time wouldn't normally be in past, it still races,
> as describe above. In that case, the math here underflows alarm_secs,
> so it probably still makes sense to check.

I can't believe you can possibly have more than one second between the
check in the core and the check in the driver, it doesn't make much
sense to check, even in the current state of the core.

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

Re: [PATCH] rtc: st-lpc: remove unnecessary check

[Trent Piepho]

On Wed, 2019-05-01 at 16:25 +0200, Alexandre Belloni wrote:
> On 30/04/2019 22:31:19+0000, Trent Piepho wrote:
> > On Tue, 2019-04-30 at 22:18 +0200, Alexandre Belloni wrote:
> > > The RTC core already ensures the alarm is set to a time in the future, it
> > > is not necessary to check again in the driver.
> > 
> > My reading of the rtc core code is that it checks if the alarm is in
> > the future *twice* before handing off the set call to the driver, which
> > possibly checks a 3rd time (as seen here).
> > 
> > However, all these checks are done *before* setting the alarm.  It
> > still possible to have a race and set the alarm after the time has
> > already passed, in which case the alarm will never fire.
> > 
> 
> I agree the core need to handle that possible race better and this is
> something I'm planning to work on.
> 
> > The way to fix the race would be to have the driver check the alarm
> > *after* setting it.  In precisely this order, do these steps:
> > 
> > 1. Set alarm in RTC, to Talarm
> > 2. Get time from RTC, as Tcurrent
> > 3. Get alarm status from RTC
> > 
> > If Talarm < Tcurrent, alarm was set to future time, no error
> 
> This should be Talarm > Tcurrent, right?

Yes.  I wrote that backward.

> > Else
> >   If status == fired, alarm was set and has since fired, no error
> >   Else status == not fired, alarm was set in past, EINVAL
> > 
> > This should be race free.
> > 
> > 
> > >  
> > > -	/* Invalid alarm time */
> > > -	if (now_secs > alarm_secs)
> > > -		return -EINVAL;
> > > -
> > >  	memcpy(&rtc->alarm, t, sizeof(struct rtc_wkalrm));
> > >  
> > >  	/* Now many secs to fire */
> > 
> >         alarm_secs -= now_secs;
> >         lpa = (unsigned long long)alarm_secs * rtc->clkrate;
> > 
> > While it's true the time wouldn't normally be in past, it still races,
> > as describe above. In that case, the math here underflows alarm_secs,
> > so it probably still makes sense to check.
> 
> I can't believe you can possibly have more than one second between the
> check in the core and the check in the driver, it doesn't make much
> sense to check, even in the current state of the core.

It's certainly possible to have multiple seconds pass.  For an external
device over SPI or I2C, one has to wait for the bus to become free. 
And on SPI that requires the kernel thread running the bus to be 
scheduled.  Just put in some real-time tasks and maybe a big transfer
to a flash chip and it could be a while before that happens.

I don't think this device has that issue as I don't think it's
external.  And ever for a device on an external bus, delays > 1 second
are unlikely.  Possible, but unlikely.

You can also get them when Linux is running under a hypervisor, i.e. a
Linux VM.  But also something like an NMI and ACPI BIOS.  If the Linux
guest is not scheduled to run for while anything that is supposed to be
based on real time, like the value returned by an RTC, will still
advance.  It is possible that multiple seconds elapse from the guest
CPU executing one instruction to the next.

But even ignoring that, does it require > 1 second to elapse.  Can't it
happen when the clock ticks from one second to the next, which happens
effectively instantly?

If the time from the check to the time when the alarm is set is 1
microsecond, and the time this call to set the alarm is made is
randomly done and not synchronized to the RTC, then isn't there a 1 out
of 1 million chance (1 microsecond / 1 second), that the once per
second clock tick will hit our 1 us window?

Re: [PATCH] rtc: st-lpc: remove unnecessary check

[Alexandre Belloni]

On 01/05/2019 17:11:44+0000, Trent Piepho wrote:
> > I can't believe you can possibly have more than one second between the
> > check in the core and the check in the driver, it doesn't make much
> > sense to check, even in the current state of the core.
> 
> It's certainly possible to have multiple seconds pass.  For an external
> device over SPI or I2C, one has to wait for the bus to become free. 
> And on SPI that requires the kernel thread running the bus to be 
> scheduled.  Just put in some real-time tasks and maybe a big transfer
> to a flash chip and it could be a while before that happens.
> 
> I don't think this device has that issue as I don't think it's
> external.  And ever for a device on an external bus, delays > 1 second
> are unlikely.  Possible, but unlikely.
> 
> You can also get them when Linux is running under a hypervisor, i.e. a
> Linux VM.  But also something like an NMI and ACPI BIOS.  If the Linux
> guest is not scheduled to run for while anything that is supposed to be
> based on real time, like the value returned by an RTC, will still
> advance.  It is possible that multiple seconds elapse from the guest
> CPU executing one instruction to the next.
> 
> But even ignoring that, does it require > 1 second to elapse.  Can't it
> happen when the clock ticks from one second to the next, which happens
> effectively instantly?
> 
> If the time from the check to the time when the alarm is set is 1
> microsecond, and the time this call to set the alarm is made is
> randomly done and not synchronized to the RTC, then isn't there a 1 out
> of 1 million chance (1 microsecond / 1 second), that the once per
> second clock tick will hit our 1 us window?

No, let's say you want Talarm == Tcurrent + 1, if the core check happens
right before the next second, then you necessarily end up with
Talarm == Tcurrent after the check. This means that you now have one
second before the time read in st-lpc to avoid the
alarm_secs -= now_secs; underflow.

Obviously, in that case, you are likely to miss the alarm but this is as
likely to happen with the check that is in the driver. This check
doesn't provide anything but a false sense of security.

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