[PATCH] rtc: class: Do not inject sleep time without a suspend snapshot

[PATCH] rtc: class: Do not inject sleep time without a suspend snapshot

[Alperen Kılıç]

rtc_suspend() returns early when timekeeping_rtc_skipsuspend() is true,
which is the case on every system with a persistent clock. old_rtc and
old_system are then never written and stay zero.

rtc_resume() is skipped under a different condition,
timekeeping_rtc_skipresume(), which is only true once
timekeeping_resume() has injected sleep time. timekeeping_resume() does
that if a nonstop clocksource reports elapsed time or if the persistent
clock is ahead of timekeeping_suspend_time. If neither holds, it injects
nothing and rtc_resume() runs. With old_rtc and old_system being zero,
rtc_resume() computes

	sleep_time = new_rtc - new_system

and injects it when it is not negative. For an RTC kept in UTC this is
about zero. For an RTC kept in local time east of UTC it is the UTC
offset, so CLOCK_REALTIME and CLOCK_BOOTTIME jump ahead by that amount.

This happens with suspend-to-idle on x86. The persistent clock is the
CMOS RTC with a resolution of one second, and the TSC is only flagged
CLOCK_SOURCE_SUSPEND_NONSTOP on CPUs with X86_FEATURE_NONSTOP_TSC_S3.
timekeeping_suspend() and timekeeping_resume() are called for every pass
through the s2idle loop, and when the last pass freezes timekeeping for
less than a second, the persistent clock may not have advanced past
timekeeping_suspend_time.

Seen on a Lenovo ThinkBook 16p G5 (Core i9-14900HX, s2idle only) with the
RTC in local time at UTC+3: on some resumes the system clock jumped
three hours ahead and chronyd then slewed it back for more than a day.
A kprobe on timekeeping_inject_sleeptime64() shows the call coming from
rtc_resume() with a delta of 10798 seconds.

Record in rtc_suspend() whether the snapshot was taken and return early
from rtc_resume() when it was not. This also covers the case where
rtc_suspend() failed to read the RTC. The device name check moves in
front of timekeeping_rtc_skipsuspend() so that the flag is cleared on
every suspend of the hctosys device.

A freeze too short to be seen by the persistent clock is not accounted
after this change. With the RTC in UTC the injected value used to be the
sub-second difference between the RTC and the system time, not the time
spent suspended.

Tested on v7.3-rc5 with a script that shifts the system clock by less
than a second relative to the RTC and then suspends to idle with a wake
alarm a few seconds ahead, 30 times. Unpatched, 4 cycles jumped by the
UTC offset, each of them with timekeeping frozen for less than 0.9
seconds. Patched, no cycle jumped although 6 of them met that condition.

Fixes: 0fa88cb4b82b ("time, drivers/rtc: Don't bother with rtc_resume() for the nonstop clocksource")
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2543517
Cc: stable@vger.kernel.org
Signed-off-by: Alperen Kılıç <sabri.alperen03@gmail.com>
---

Notes:
    The jump was found and measured on my own laptop (suspend hook log in
    the Bugzilla report). An LLM assisted in analyzing the RTC and timekeeping
    resume path, writing the reproducer script and drafting this patch and
    its changelog. I reviewed the change and ran the reproducer on unpatched
    and patched v7.3-rc5 builds myself.

 drivers/rtc/class.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 01ba04028..c1a949252 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -96,6 +96,8 @@ static void rtc_hctosys(struct rtc_device *rtc)
  */
 
 static struct timespec64 old_rtc, old_system, old_delta;
+/* Set when rtc_suspend() took the snapshot rtc_resume() relies on */
+static bool old_rtc_valid;
 
 static int rtc_suspend(struct device *dev)
 {
@@ -104,10 +106,12 @@ static int rtc_suspend(struct device *dev)
 	struct timespec64	delta, delta_delta;
 	int err;
 
-	if (timekeeping_rtc_skipsuspend())
+	if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
 		return 0;
 
-	if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
+	old_rtc_valid = false;
+
+	if (timekeeping_rtc_skipsuspend())
 		return 0;
 
 	/* snapshot the current RTC and system time at suspend*/
@@ -119,6 +123,7 @@ static int rtc_suspend(struct device *dev)
 
 	ktime_get_real_ts64(&old_system);
 	old_rtc.tv_sec = rtc_tm_to_time64(&tm);
+	old_rtc_valid = true;
 
 	/*
 	 * To avoid drift caused by repeated suspend/resumes,
@@ -153,6 +158,18 @@ static int rtc_resume(struct device *dev)
 	if (timekeeping_rtc_skipresume())
 		return 0;
 
+	/*
+	 * rtc_suspend() takes no snapshot when the persistent clock is in
+	 * charge of the sleep time or when the RTC could not be read. The
+	 * timekeeping core still asks for sleep time if the persistent clock
+	 * did not advance, which happens when the system was suspended for
+	 * less than a second. Without a snapshot there is nothing to compute
+	 * the sleep time from. Using the stale old_rtc and old_system would
+	 * inject the offset between the RTC and system time.
+	 */
+	if (!old_rtc_valid)
+		return 0;
+
 	rtc_hctosys_ret = -ENODEV;
 	if (strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE) != 0)
 		return 0;

base-commit: e767a4ea70a3992c37ed604157d32f0dfbf9b1e3
-- 
2.55.0

Re: [PATCH] rtc: class: Do not inject sleep time without a suspend snapshot

[sashiko-bot]

> rtc_suspend() returns early when timekeeping_rtc_skipsuspend() is true,
> which is the case on every system with a persistent clock. old_rtc and
> old_system are then never written and stay zero.
> 
> rtc_resume() is skipped under a different condition,
> timekeeping_rtc_skipresume(), which is only true once
> timekeeping_resume() has injected sleep time. timekeeping_resume() does
> that if a nonstop clocksource reports elapsed time or if the persistent
> clock is ahead of timekeeping_suspend_time. If neither holds, it injects
> nothing and rtc_resume() runs. With old_rtc and old_system being zero,
> rtc_resume() computes
> 
> 	sleep_time = new_rtc - new_system
> 
> and injects it when it is not negative. For an RTC kept in UTC this is
> [ ... ]
> 
> Fixes: 0fa88cb4b82b ("time, drivers/rtc: Don't bother with rtc_resume() for the nonstop clocksource")
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2543517
> Cc: stable@vger.kernel.org
> Signed-off-by: Alperen Kılıç <sabri.alperen03@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003163138.14221-1-sabri.alperen03@gmail.com?part=1