[PATCH] ntp: Make sure RTC is synchronized when time goes backwards

[PATCH] ntp: Make sure RTC is synchronized when time goes backwards

[Benjamin ROBIN]

The "sync_hw_clock" is normally called every 11 minutes when time is
synchronized. This issue is that this periodic timer uses the REALTIME
clock, so when time moves backwards (the NTP server jumps into the past),
the next call to "sync_hw_clock" could be realized after a very long
period.

A normal NTP server should not jump in the past like that, but it is
possible... Another way to reproduce this issue is using phc2sys to
synchronize the REALTIME clock with for example an IRIG timecode with
the source always starting at the same date (not synchronized).

This patch cancels the periodic timer on a time jump (ADJ_SETOFFSET).
The timer will be relaunched at the end of "do_adjtimex" if NTP is still
considered synced. Otherwise the timer will be relaunched later when NTP
is synced. This way, when the time is synchronized again, the RTC is
updated after less than 2 seconds.

Signed-off-by: Benjamin ROBIN <dev@benjarobin.fr>
---
 kernel/time/ntp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 8d2dd214ec68..5c8dd92cf012 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -751,6 +751,9 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
 
 	if (txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET))
 		ntp_update_frequency();
+
+	if (txc->modes & ADJ_SETOFFSET)
+		hrtimer_cancel(&sync_hrtimer);
 }
 
 
-- 
2.45.2

Re: [PATCH] ntp: Make sure RTC is synchronized when time goes backwards

[Thomas Gleixner]

On Sat, Sep 07 2024 at 21:09, Benjamin ROBIN wrote:
> The "sync_hw_clock" is normally called every 11 minutes when time is

s/The "sync_hw_clock"/sync_hw_clock()/

See: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#function-references-in-changelogs

> synchronized. This issue is that this periodic timer uses the REALTIME
> clock, so when time moves backwards (the NTP server jumps into the past),
> the next call to "sync_hw_clock" could be realized after a very long

s/the next .../the timer expires late./

And then please explain what the consequence is when it expires
late. See the changelog section of the above link.

> period.

Cute.

> A normal NTP server should not jump in the past like that, but it is
> possible... Another way to reproduce this issue is using phc2sys to
> synchronize the REALTIME clock with for example an IRIG timecode with
> the source always starting at the same date (not synchronized).
>
> This patch cancels the periodic timer on a time jump (ADJ_SETOFFSET).

s/This patch cancels/Cancel/

For explanation:
# git grep 'This patch' Documentation/process 

> The timer will be relaunched at the end of "do_adjtimex" if NTP is still
> considered synced. Otherwise the timer will be relaunched later when NTP
> is synced. This way, when the time is synchronized again, the RTC is
> updated after less than 2 seconds.
>
> Signed-off-by: Benjamin ROBIN <dev@benjarobin.fr>
> ---
>  kernel/time/ntp.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index 8d2dd214ec68..5c8dd92cf012 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -751,6 +751,9 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
>  
>  	if (txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET))
>  		ntp_update_frequency();
> +
> +	if (txc->modes & ADJ_SETOFFSET)
> +		hrtimer_cancel(&sync_hrtimer);

Did you test this with lockdep enabled?

The caller holds timekeeping_lock and has the time keeper sequence write
held. There is an existing dependency chain which is invers. Assume the
sync_hrtimer is queued on a different CPU, CPU1 in this example:

CPU 0                                       CPU1

lock(&timekeeper_lock);                     lock_hrtimer_base(CPU1);    

write_seqcount_begin(&tk_core.seq); <- Makes tk_core.seq odd

__do_adjtimex()
  process_adjtimex_modes()                  base->get_time()
    hrtimer_cancel()                          do {
      hrtimer_try_to_cancel()                       seq = read_seqcount_begin(&tk_core.seq);
        lock_hrtimer_base(CPU1);                          ^^^
        ^^^ deadlock                                      Spin waits for tk_core.seq
                                                          to become even

You can do that cancel only outside of timekeeper_lock:

--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -2426,6 +2426,7 @@ int do_adjtimex(struct __kernel_timex *t
 {
 	struct timekeeper *tk = &tk_core.timekeeper;
 	struct audit_ntp_data ad;
+	bool offset_set = false;
 	bool clock_set = false;
 	struct timespec64 ts;
 	unsigned long flags;
@@ -2448,6 +2449,7 @@ int do_adjtimex(struct __kernel_timex *t
 		if (ret)
 			return ret;
 
+		offset_set = delta.tv_sec < 0;
 		audit_tk_injoffset(delta);
 	}
 
@@ -2481,7 +2483,7 @@ int do_adjtimex(struct __kernel_timex *t
 	if (clock_set)
 		clock_was_set(CLOCK_REALTIME);
 
-	ntp_notify_cmos_timer();
+	ntp_notify_cmos_timer(offset_set);
 
 	return ret;
 }

Now you can fix that up in ntp_notify_cmos_timer() which is outside of
the timekeeper_lock held region for the very same reason and it's the
proper place to do that.

Thanks,

        tglx

Re: [PATCH] ntp: Make sure RTC is synchronized when time goes backwards

[Benjamin ROBIN]

On Sat, Sep 07, 2024 at 11:42:55PM GMT, Thomas Gleixner wrote:
> s/The "sync_hw_clock"/sync_hw_clock()/
> 
> See: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#function-references-in-changelogs
> 
> s/the next .../the timer expires late./
> 
> And then please explain what the consequence is when it expires
> late. See the changelog section of the above link.

> s/This patch cancels/Cancel/
> 
> For explanation:
> # git grep 'This patch' Documentation/process 

Thank you for your remarks and the time spent to review this commit!
 
> Did you test this with lockdep enabled?

I did not... Indeed this is an huge mistake. Sorry!
 
> The caller holds timekeeping_lock and has the time keeper sequence write
> held. There is an existing dependency chain which is invers. Assume the
> sync_hrtimer is queued on a different CPU, CPU1 in this example:
> 
> CPU 0                                       CPU1
> 
> lock(&timekeeper_lock);                     lock_hrtimer_base(CPU1);    
> 
> write_seqcount_begin(&tk_core.seq); <- Makes tk_core.seq odd
> 
> __do_adjtimex()
>   process_adjtimex_modes()                  base->get_time()
>     hrtimer_cancel()                          do {
>       hrtimer_try_to_cancel()                       seq = read_seqcount_begin(&tk_core.seq);
>         lock_hrtimer_base(CPU1);                          ^^^
>         ^^^ deadlock                                      Spin waits for tk_core.seq
>                                                           to become even
> 
> You can do that cancel only outside of timekeeper_lock:

Again thank you for the time spent to explain this in some much detail.
You did not have to. This is really appreciated.

> Now you can fix that up in ntp_notify_cmos_timer() which is outside of
> the timekeeper_lock held region for the very same reason and it's the
> proper place to do that.

I will cancel the timer even for time jump in the future, everything will be 
explained in the commit message. Will see if you are OK with that.

> 
> Thanks,
> 
>         tglx

Thanks, Benjamin

[PATCH v2] ntp: Make sure RTC is synchronized when time goes backwards

[Benjamin ROBIN]

sync_hw_clock() is normally called every 11 minutes when time is
synchronized. This issue is that this periodic timer uses the REALTIME
clock, so when time moves backwards (the NTP server jumps into the past),
the timer expires late.

If the timer expires late, which can be days later, the RTC will no longer
be updated, which is an issue if the device is abruptly powered OFF during
this period. When the device will restart (when powered ON), it will have
the date prior to the ADJ_SETOFFSET call.

A normal NTP server should not jump in the past like that, but it is
possible... Another way of reproducing this issue is to use phc2sys to
synchronize the REALTIME clock with, for example, an IRIG timecode with
the source always starting at the same date (not synchronized).

Also, if the time jump in the future by less than 11 minutes, the RTC may
not be updated immediately (minor issue). Consider the following scenario:
 - Time is synchronized, and sync_hw_clock() was just called (the timer
   expires in 11 minutes).
 - A time jump is realized in the future by a couple of minutes.
 - The time is synchronized again.
 - Users may expect that RTC to be updated as soon as possible, and not
   after 11 minutes (for the same reason, if a power loss occurs in this
   period).

Cancel periodic timer on any time jump (ADJ_SETOFFSET) greater than or
equal to 1s. The timer will be relaunched at the end of do_adjtimex() if
NTP is still considered synced. Otherwise the timer will be relaunched
later when NTP is synced. This way, when the time is synchronized again,
the RTC is updated after less than 2 seconds.

Signed-off-by: Benjamin ROBIN <dev@benjarobin.fr>
---
v2: Improve commit message (takes into account tglx remarks).
    Cancel timer outside timekeeper_lock, from ntp_notify_cmos_timer()
    as recommended. Cancel timer on negative or positive jump.

 kernel/time/ntp.c          | 10 +++++++++-
 kernel/time/ntp_internal.h |  4 ++--
 kernel/time/timekeeping.c  |  4 +++-
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 8d2dd214ec68..802b336f4b8c 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -660,8 +660,16 @@ static void sync_hw_clock(struct work_struct *work)
 	sched_sync_hw_clock(offset_nsec, res != 0);
 }

-void ntp_notify_cmos_timer(void)
+void ntp_notify_cmos_timer(bool offset_set)
 {
+	/*
+	 * If the time jumped (using ADJ_SETOFFSET) cancels sync timer,
+	 * which may have been running if the time was synchronized
+	 * prior to the ADJ_SETOFFSET call.
+	 */
+	if (offset_set)
+		hrtimer_cancel(&sync_hrtimer);
+
 	/*
 	 * When the work is currently executed but has not yet the timer
 	 * rearmed this queues the work immediately again. No big issue,
diff --git a/kernel/time/ntp_internal.h b/kernel/time/ntp_internal.h
index 23d1b74c3065..5a633dce9057 100644
--- a/kernel/time/ntp_internal.h
+++ b/kernel/time/ntp_internal.h
@@ -14,9 +14,9 @@ extern int __do_adjtimex(struct __kernel_timex *txc,
 extern void __hardpps(const struct timespec64 *phase_ts, const struct timespec64 *raw_ts);

 #if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
-extern void ntp_notify_cmos_timer(void);
+extern void ntp_notify_cmos_timer(bool offset_set);
 #else
-static inline void ntp_notify_cmos_timer(void) { }
+static inline void ntp_notify_cmos_timer(bool offset_set) { }
 #endif

 #endif /* _LINUX_NTP_INTERNAL_H */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 5391e4167d60..7e6f409bf311 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -2553,6 +2553,7 @@ int do_adjtimex(struct __kernel_timex *txc)
 {
 	struct timekeeper *tk = &tk_core.timekeeper;
 	struct audit_ntp_data ad;
+	bool offset_set = false;
 	bool clock_set = false;
 	struct timespec64 ts;
 	unsigned long flags;
@@ -2575,6 +2576,7 @@ int do_adjtimex(struct __kernel_timex *txc)
 		if (ret)
 			return ret;

+		offset_set = delta.tv_sec != 0;
 		audit_tk_injoffset(delta);
 	}

@@ -2608,7 +2610,7 @@ int do_adjtimex(struct __kernel_timex *txc)
 	if (clock_set)
 		clock_was_set(CLOCK_SET_WALL);

-	ntp_notify_cmos_timer();
+	ntp_notify_cmos_timer(offset_set);

 	return ret;
 }
--
2.45.2

Re: [PATCH] ntp: Make sure RTC is synchronized when time goes backwards

[kernel test robot]

Hi Benjamin,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/timers/core]
[also build test ERROR on linus/master v6.11-rc6 next-20240906]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-ROBIN/ntp-Make-sure-RTC-is-synchronized-when-time-goes-backwards/20240908-032754
base:   tip/timers/core
patch link:    https://lore.kernel.org/r/20240907190900.55421-1-dev%40benjarobin.fr
patch subject: [PATCH] ntp: Make sure RTC is synchronized when time goes backwards
config: alpha-allnoconfig (https://download.01.org/0day-ci/archive/20240908/202409082008.BODTFEfx-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240908/202409082008.BODTFEfx-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409082008.BODTFEfx-lkp@intel.com/

All errors (new ones prefixed by >>):

   kernel/time/ntp.c: In function 'process_adjtimex_modes':
>> kernel/time/ntp.c:757:33: error: 'sync_hrtimer' undeclared (first use in this function)
     757 |                 hrtimer_cancel(&sync_hrtimer);
         |                                 ^~~~~~~~~~~~
   kernel/time/ntp.c:757:33: note: each undeclared identifier is reported only once for each function it appears in


vim +/sync_hrtimer +757 kernel/time/ntp.c

   707	
   708	
   709	static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
   710						  s32 *time_tai)
   711	{
   712		if (txc->modes & ADJ_STATUS)
   713			process_adj_status(txc);
   714	
   715		if (txc->modes & ADJ_NANO)
   716			time_status |= STA_NANO;
   717	
   718		if (txc->modes & ADJ_MICRO)
   719			time_status &= ~STA_NANO;
   720	
   721		if (txc->modes & ADJ_FREQUENCY) {
   722			time_freq = txc->freq * PPM_SCALE;
   723			time_freq = min(time_freq, MAXFREQ_SCALED);
   724			time_freq = max(time_freq, -MAXFREQ_SCALED);
   725			/* update pps_freq */
   726			pps_set_freq(time_freq);
   727		}
   728	
   729		if (txc->modes & ADJ_MAXERROR)
   730			time_maxerror = txc->maxerror;
   731	
   732		if (txc->modes & ADJ_ESTERROR)
   733			time_esterror = txc->esterror;
   734	
   735		if (txc->modes & ADJ_TIMECONST) {
   736			time_constant = txc->constant;
   737			if (!(time_status & STA_NANO))
   738				time_constant += 4;
   739			time_constant = min(time_constant, (long)MAXTC);
   740			time_constant = max(time_constant, 0l);
   741		}
   742	
   743		if (txc->modes & ADJ_TAI &&
   744				txc->constant >= 0 && txc->constant <= MAX_TAI_OFFSET)
   745			*time_tai = txc->constant;
   746	
   747		if (txc->modes & ADJ_OFFSET)
   748			ntp_update_offset(txc->offset);
   749	
   750		if (txc->modes & ADJ_TICK)
   751			tick_usec = txc->tick;
   752	
   753		if (txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET))
   754			ntp_update_frequency();
   755	
   756		if (txc->modes & ADJ_SETOFFSET)
 > 757			hrtimer_cancel(&sync_hrtimer);
   758	}
   759	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] ntp: Make sure RTC is synchronized when time goes backwards

[kernel test robot]

Hi Benjamin,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/timers/core]
[also build test ERROR on linus/master v6.11-rc6 next-20240906]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-ROBIN/ntp-Make-sure-RTC-is-synchronized-when-time-goes-backwards/20240908-032754
base:   tip/timers/core
patch link:    https://lore.kernel.org/r/20240907190900.55421-1-dev%40benjarobin.fr
patch subject: [PATCH] ntp: Make sure RTC is synchronized when time goes backwards
config: arm-randconfig-r071-20240908 (https://download.01.org/0day-ci/archive/20240908/202409082049.yA7d6Zmw-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 05f5a91d00b02f4369f46d076411c700755ae041)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240908/202409082049.yA7d6Zmw-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409082049.yA7d6Zmw-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from kernel/time/ntp.c:17:
   In file included from include/linux/mm.h:2228:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> kernel/time/ntp.c:757:19: error: use of undeclared identifier 'sync_hrtimer'
     757 |                 hrtimer_cancel(&sync_hrtimer);
         |                                 ^
   1 warning and 1 error generated.


vim +/sync_hrtimer +757 kernel/time/ntp.c

   707	
   708	
   709	static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
   710						  s32 *time_tai)
   711	{
   712		if (txc->modes & ADJ_STATUS)
   713			process_adj_status(txc);
   714	
   715		if (txc->modes & ADJ_NANO)
   716			time_status |= STA_NANO;
   717	
   718		if (txc->modes & ADJ_MICRO)
   719			time_status &= ~STA_NANO;
   720	
   721		if (txc->modes & ADJ_FREQUENCY) {
   722			time_freq = txc->freq * PPM_SCALE;
   723			time_freq = min(time_freq, MAXFREQ_SCALED);
   724			time_freq = max(time_freq, -MAXFREQ_SCALED);
   725			/* update pps_freq */
   726			pps_set_freq(time_freq);
   727		}
   728	
   729		if (txc->modes & ADJ_MAXERROR)
   730			time_maxerror = txc->maxerror;
   731	
   732		if (txc->modes & ADJ_ESTERROR)
   733			time_esterror = txc->esterror;
   734	
   735		if (txc->modes & ADJ_TIMECONST) {
   736			time_constant = txc->constant;
   737			if (!(time_status & STA_NANO))
   738				time_constant += 4;
   739			time_constant = min(time_constant, (long)MAXTC);
   740			time_constant = max(time_constant, 0l);
   741		}
   742	
   743		if (txc->modes & ADJ_TAI &&
   744				txc->constant >= 0 && txc->constant <= MAX_TAI_OFFSET)
   745			*time_tai = txc->constant;
   746	
   747		if (txc->modes & ADJ_OFFSET)
   748			ntp_update_offset(txc->offset);
   749	
   750		if (txc->modes & ADJ_TICK)
   751			tick_usec = txc->tick;
   752	
   753		if (txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET))
   754			ntp_update_frequency();
   755	
   756		if (txc->modes & ADJ_SETOFFSET)
 > 757			hrtimer_cancel(&sync_hrtimer);
   758	}
   759	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3] ntp: Make sure RTC is synchronized when time goes backwards

[Benjamin ROBIN]

sync_hw_clock() is normally called every 11 minutes when time is
synchronized. This issue is that this periodic timer uses the REALTIME
clock, so when time moves backwards (the NTP server jumps into the past),
the timer expires late.

If the timer expires late, which can be days later, the RTC will no longer
be updated, which is an issue if the device is abruptly powered OFF during
this period. When the device will restart (when powered ON), it will have
the date prior to the ADJ_SETOFFSET call.

A normal NTP server should not jump in the past like that, but it is
possible... Another way of reproducing this issue is to use phc2sys to
synchronize the REALTIME clock with, for example, an IRIG timecode with
the source always starting at the same date (not synchronized).

Also, if the time jump in the future by less than 11 minutes, the RTC may
not be updated immediately (minor issue). Consider the following scenario:
 - Time is synchronized, and sync_hw_clock() was just called (the timer
   expires in 11 minutes).
 - A time jump is realized in the future by a couple of minutes.
 - The time is synchronized again.
 - Users may expect that RTC to be updated as soon as possible, and not
   after 11 minutes (for the same reason, if a power loss occurs in this
   period).

Cancel periodic timer on any time jump (ADJ_SETOFFSET) greater than or
equal to 1s. The timer will be relaunched at the end of do_adjtimex() if
NTP is still considered synced. Otherwise the timer will be relaunched
later when NTP is synced. This way, when the time is synchronized again,
the RTC is updated after less than 2 seconds.

Signed-off-by: Benjamin ROBIN <dev@benjarobin.fr>
---
v2: Improve commit message (takes into account tglx remarks).
    Cancel timer outside timekeeper_lock, from ntp_notify_cmos_timer()
    as recommended. Cancel timer on negative or positive jump.
v3: No changes. Resent since lore.kernel.org did not pick it up.
    Also, to indicates that it should also fix the build issue discovered
    by kernel test robot. v1 was really broken, sorry about that.

 kernel/time/ntp.c          | 10 +++++++++-
 kernel/time/ntp_internal.h |  4 ++--
 kernel/time/timekeeping.c  |  4 +++-
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 8d2dd214ec68..802b336f4b8c 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -660,8 +660,16 @@ static void sync_hw_clock(struct work_struct *work)
 	sched_sync_hw_clock(offset_nsec, res != 0);
 }
 
-void ntp_notify_cmos_timer(void)
+void ntp_notify_cmos_timer(bool offset_set)
 {
+	/*
+	 * If the time jumped (using ADJ_SETOFFSET) cancels sync timer,
+	 * which may have been running if the time was synchronized
+	 * prior to the ADJ_SETOFFSET call.
+	 */
+	if (offset_set)
+		hrtimer_cancel(&sync_hrtimer);
+
 	/*
 	 * When the work is currently executed but has not yet the timer
 	 * rearmed this queues the work immediately again. No big issue,
diff --git a/kernel/time/ntp_internal.h b/kernel/time/ntp_internal.h
index 23d1b74c3065..5a633dce9057 100644
--- a/kernel/time/ntp_internal.h
+++ b/kernel/time/ntp_internal.h
@@ -14,9 +14,9 @@ extern int __do_adjtimex(struct __kernel_timex *txc,
 extern void __hardpps(const struct timespec64 *phase_ts, const struct timespec64 *raw_ts);
 
 #if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
-extern void ntp_notify_cmos_timer(void);
+extern void ntp_notify_cmos_timer(bool offset_set);
 #else
-static inline void ntp_notify_cmos_timer(void) { }
+static inline void ntp_notify_cmos_timer(bool offset_set) { }
 #endif
 
 #endif /* _LINUX_NTP_INTERNAL_H */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 5391e4167d60..7e6f409bf311 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -2553,6 +2553,7 @@ int do_adjtimex(struct __kernel_timex *txc)
 {
 	struct timekeeper *tk = &tk_core.timekeeper;
 	struct audit_ntp_data ad;
+	bool offset_set = false;
 	bool clock_set = false;
 	struct timespec64 ts;
 	unsigned long flags;
@@ -2575,6 +2576,7 @@ int do_adjtimex(struct __kernel_timex *txc)
 		if (ret)
 			return ret;
 
+		offset_set = delta.tv_sec != 0;
 		audit_tk_injoffset(delta);
 	}
 
@@ -2608,7 +2610,7 @@ int do_adjtimex(struct __kernel_timex *txc)
 	if (clock_set)
 		clock_was_set(CLOCK_SET_WALL);
 
-	ntp_notify_cmos_timer();
+	ntp_notify_cmos_timer(offset_set);
 
 	return ret;
 }
-- 
2.45.2

[tip: timers/core] ntp: Make sure RTC is synchronized when time goes backwards

[tip-bot2 for Benjamin ROBIN]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     35b603f8a78b0bd51566db277c4f7b56b3ff6bac
Gitweb:        https://git.kernel.org/tip/35b603f8a78b0bd51566db277c4f7b56b3ff6bac
Author:        Benjamin ROBIN <dev@benjarobin.fr>
AuthorDate:    Sun, 08 Sep 2024 16:08:36 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 10 Sep 2024 13:50:40 +02:00

ntp: Make sure RTC is synchronized when time goes backwards

sync_hw_clock() is normally called every 11 minutes when time is
synchronized. This issue is that this periodic timer uses the REALTIME
clock, so when time moves backwards (the NTP server jumps into the past),
the timer expires late.

If the timer expires late, which can be days later, the RTC will no longer
be updated, which is an issue if the device is abruptly powered OFF during
this period. When the device will restart (when powered ON), it will have
the date prior to the ADJ_SETOFFSET call.

A normal NTP server should not jump in the past like that, but it is
possible... Another way of reproducing this issue is to use phc2sys to
synchronize the REALTIME clock with, for example, an IRIG timecode with
the source always starting at the same date (not synchronized).

Also, if the time jump in the future by less than 11 minutes, the RTC may
not be updated immediately (minor issue). Consider the following scenario:
 - Time is synchronized, and sync_hw_clock() was just called (the timer
   expires in 11 minutes).
 - A time jump is realized in the future by a couple of minutes.
 - The time is synchronized again.
 - Users may expect that RTC to be updated as soon as possible, and not
   after 11 minutes (for the same reason, if a power loss occurs in this
   period).

Cancel periodic timer on any time jump (ADJ_SETOFFSET) greater than or
equal to 1s. The timer will be relaunched at the end of do_adjtimex() if
NTP is still considered synced. Otherwise the timer will be relaunched
later when NTP is synced. This way, when the time is synchronized again,
the RTC is updated after less than 2 seconds.

Signed-off-by: Benjamin ROBIN <dev@benjarobin.fr>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/all/20240908140836.203911-1-dev@benjarobin.fr

---
 kernel/time/ntp.c          | 10 +++++++++-
 kernel/time/ntp_internal.h |  4 ++--
 kernel/time/timekeeping.c  |  4 +++-
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 8d2dd21..802b336 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -660,9 +660,17 @@ rearm:
 	sched_sync_hw_clock(offset_nsec, res != 0);
 }
 
-void ntp_notify_cmos_timer(void)
+void ntp_notify_cmos_timer(bool offset_set)
 {
 	/*
+	 * If the time jumped (using ADJ_SETOFFSET) cancels sync timer,
+	 * which may have been running if the time was synchronized
+	 * prior to the ADJ_SETOFFSET call.
+	 */
+	if (offset_set)
+		hrtimer_cancel(&sync_hrtimer);
+
+	/*
 	 * When the work is currently executed but has not yet the timer
 	 * rearmed this queues the work immediately again. No big issue,
 	 * just a pointless work scheduled.
diff --git a/kernel/time/ntp_internal.h b/kernel/time/ntp_internal.h
index 23d1b74..5a633dc 100644
--- a/kernel/time/ntp_internal.h
+++ b/kernel/time/ntp_internal.h
@@ -14,9 +14,9 @@ extern int __do_adjtimex(struct __kernel_timex *txc,
 extern void __hardpps(const struct timespec64 *phase_ts, const struct timespec64 *raw_ts);
 
 #if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
-extern void ntp_notify_cmos_timer(void);
+extern void ntp_notify_cmos_timer(bool offset_set);
 #else
-static inline void ntp_notify_cmos_timer(void) { }
+static inline void ntp_notify_cmos_timer(bool offset_set) { }
 #endif
 
 #endif /* _LINUX_NTP_INTERNAL_H */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 5391e41..7e6f409 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -2553,6 +2553,7 @@ int do_adjtimex(struct __kernel_timex *txc)
 {
 	struct timekeeper *tk = &tk_core.timekeeper;
 	struct audit_ntp_data ad;
+	bool offset_set = false;
 	bool clock_set = false;
 	struct timespec64 ts;
 	unsigned long flags;
@@ -2575,6 +2576,7 @@ int do_adjtimex(struct __kernel_timex *txc)
 		if (ret)
 			return ret;
 
+		offset_set = delta.tv_sec != 0;
 		audit_tk_injoffset(delta);
 	}
 
@@ -2608,7 +2610,7 @@ int do_adjtimex(struct __kernel_timex *txc)
 	if (clock_set)
 		clock_was_set(CLOCK_SET_WALL);
 
-	ntp_notify_cmos_timer();
+	ntp_notify_cmos_timer(offset_set);
 
 	return ret;
 }