Re: __rtc_read_alarm missing month/year field bug?

[Joe Lawrence]

On 06/20/2016 12:06 PM, Joe Lawrence wrote:
> Hello Alessandro and Alexandre,
> 
> I noticed an interesting cmos_rtc.rtc.aie_timer on a Stratus machine
> running the 4.6 kernel, with an expiration time that puts the alarm way
> out into next year.  This is easily reproducible on this machine by
> setting a wakealarm sometime in the near future, then rebooting.
> 
> From a fresh boot:
> 
>   % cat /proc/driver/rtc
>   rtc_time        : 17:55:10
>   rtc_date        : 2016-06-09
>   alrm_time       : 14:04:37
>   alrm_date       : 2017-06-09         << 2017 ?
>   alarm_IRQ       : no
>   alrm_pending    : no
>   update IRQ enabled      : no
>   periodic IRQ enabled    : no
>   periodic IRQ frequency  : 1024
>   max user IRQ frequency  : 64
>   24hr            : yes
>   periodic_IRQ    : no
>   update_IRQ      : no
>   HPET_emulated   : yes
>   BCD             : yes
>   DST_enable      : no
>   periodic_freq   : 1024
>   batt_status     : okay
> 
> 
> I added some debugging code to the kernel, saw this on the next boot:
> 
>   __rtc_read_alarm: A - alarm->time.tm_year = -1, missing = 0
>   __rtc_read_alarm: B - alarm->time.tm_year = 116, missing = 3
>   __rtc_read_alarm: C - alarm->time.tm_year = 117
> 
> 
> Corresponding to these parts of __rtc_read_alarm:
> 
>   int __rtc_read_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm)
>   ...
>   	enum { none, day, month, year } missing = none;
>   ...
>   		err = rtc_read_alarm_internal(rtc, alarm);
>   ...
>   	/* Fill in the missing alarm fields using the timestamp; we
>   	 * know there's at least one since alarm->time is invalid.
>   	 */
>   ...
>   [A]
>   	if (alarm->time.tm_year == -1) {
>   		alarm->time.tm_year = now.tm_year;
>   		if (missing == none)
>   			missing = year;
>   	}
>   [B]
>   ...
>   	switch (missing) {
>   ...
>   	/* Year rollover ... easy except for leap years! */
>   	case year:
>   		dev_dbg(&rtc->dev, "alarm rollover: %s\n", "year");
>   		do {
>   			alarm->time.tm_year++;
>   		} while (!is_leap_year(alarm->time.tm_year + 1900)
>   			&& rtc_valid_tm(&alarm->time) != 0);
>   [C]		break;
> 
> 
> I noticed that the missing year and month cases increment their
> respective time units inside a do ... while (condition) loop, pushing
> the default 'filled-in' values to now + 1.
> 
> Should this 'roll-over' code check for a valid date before incrementing
> the alarm time?  (See attached patch.)  I think this might also apply to
> a missing month field as well.
> 
> (After the patch + reboot):
> 
>   % cat /proc/driver/rtc
>   rtc_time        : 18:24:02
>   rtc_date        : 2016-06-09
>   alrm_time       : 14:04:37
>   alrm_date       : 2016-06-09
>   alarm_IRQ       : no
>   alrm_pending    : no
>   update IRQ enabled      : no
>   periodic IRQ enabled    : no
>   periodic IRQ frequency  : 1024
>   max user IRQ frequency  : 64
>   24hr            : yes
>   periodic_IRQ    : no
>   update_IRQ      : no
>   HPET_emulated   : yes
>   BCD             : yes
>   DST_enable      : no
>   periodic_freq   : 1024
>   batt_status     : okay
> 
> -- >8 --
> 
> From d6feacf20b312c8ebfee902b8b84f68c1a82f035 Mon Sep 17 00:00:00 2001
> From: Joe Lawrence <joe.lawrence@xxxxxxxxxxx>
> Date: Thu, 9 Jun 2016 14:52:28 -0400
> Subject: [PATCH] rtc: check filled-in alarm values before incrementing
> 
> In __rtc_read_alarm, check filled-in alarm->time.tm_year values (those
> not returned by the RTC and defaulted to now.tm_year) before
> incrementing them in the rollover handling case.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@xxxxxxxxxxx>
> ---
>  drivers/rtc/interface.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 9ef5f6f89f98..3098ce4167ef 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -258,10 +258,10 @@ int __rtc_read_alarm(struct rtc_device *rtc,
> struct rtc_wkalrm *alarm)
>  	/* Year rollover ... easy except for leap years! */
>  	case year:
>  		dev_dbg(&rtc->dev, "alarm rollover: %s\n", "year");
> -		do {
> +		while (!is_leap_year(alarm->time.tm_year + 1900)
> +			&& rtc_valid_tm(&alarm->time) != 0) {
>  			alarm->time.tm_year++;
> -		} while (!is_leap_year(alarm->time.tm_year + 1900)
> -			&& rtc_valid_tm(&alarm->time) != 0);
> +		}
>  		break;
> 
>  	default:
> 

Ping?  This isn't a major problem, but can setup endless RTC interrupts
under certain conditions on said hardware.


-- Joe