Re: __rtc_read_alarm missing month/year field bug?

[Alexandre Belloni]

Hi,

On 20/06/2016 at 12:06:38 -0400, 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 ?

Well, alrm_time is before rtc_time so it either already expired or is set
for the future. The code assume it is set in the future (hence the
rollover).

What you patch will do in that case is effectively always remove the
rollover.

>   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:
> -- 
> 1.8.3.1
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com