timesyncd: add maximum clock step policy - #43117
Conversation
Add MaxClockChangeSec= as an opt-in upper bound for the absolute offset accepted from a single NTP response. Responses beyond the configured limit are ignored and timesyncd moves to another server. The default remains infinity for compatibility with RTC-less systems that need a large initial correction. Closes systemd#17782.
2bac6da to
29a4730
Compare
|
Addressed the review in 29a4730:
I also converted the newly added test assertions to the current |
Claude review of PR #43117 (29a4730)Suggestions
Nits
|
| bool manager_clock_change_is_too_large(Manager *m, double offset) { | ||
| assert(m); | ||
|
|
||
| return fabs(offset) > (double) m->max_clock_change_usec / USEC_PER_SEC; |
There was a problem hiding this comment.
Claude: suggestion: When max_clock_change_usec == USEC_INFINITY (the default), this does not compare against infinity. (double) USEC_INFINITY is the finite double 1.8446744073709552e19, so the threshold becomes ~1.844e13 seconds rather than truly unbounded, and any offset above that would be rejected even under the documented "infinity" default. In practice NTP offsets are bounded well below that, so existing behavior is preserved, but relying on that implicit bound is fragile and the code no longer literally means "no limit". Consider short-circuiting explicitly, e.g. if (m->max_clock_change_usec == USEC_INFINITY) return false; before the floating-point comparison, which also sidesteps the USEC_INFINITY / USEC_PER_SEC precision detail.
| SD_BUS_PROPERTY("ServerName", "s", property_get_current_server_name, offsetof(Manager, current_server_name), 0), | ||
| SD_BUS_PROPERTY("ServerAddress", "(iay)", property_get_current_server_address, offsetof(Manager, current_server_address), 0), | ||
| SD_BUS_PROPERTY("RootDistanceMaxUSec", "t", bus_property_get_usec, offsetof(Manager, root_distance_max_usec), SD_BUS_VTABLE_PROPERTY_CONST), | ||
| SD_BUS_PROPERTY("MaxClockChangeUSec", "t", bus_property_get_usec, offsetof(Manager, max_clock_change_usec), SD_BUS_VTABLE_PROPERTY_CONST), |
There was a problem hiding this comment.
Claude: suggestion: A new read-only D-Bus property MaxClockChangeUSec is added to the manager vtable, but the generated interface documentation in man/org.freedesktop.timesync1.xml is not updated to list it. That file is auto-generated (via tools/update-dbus-docs.py) and kept committed in sync with the exported interface; sibling t-typed const properties (RootDistanceMaxUSec, PollIntervalMinUSec, etc.) all appear there. Please regenerate/update it so the introspection docs match the exported interface.
| ASSERT_TRUE(manager_clock_change_is_too_large(m, 5.001)); | ||
| ASSERT_TRUE(manager_clock_change_is_too_large(m, -5.001)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Claude: nit: The default-infinity assertion only exercises 1e9, which is far below the accidental finite threshold (~1.844e13 s) produced by (double) USEC_INFINITY / USEC_PER_SEC. It therefore does not actually verify that the default disables the check for arbitrarily large offsets. If the intent is that infinity means "never too large", consider asserting with an offset above that bound (e.g. 1e14); today such a value would incorrectly report too-large.
yuwata
left a comment
There was a problem hiding this comment.
CIs are unhappy.
Please also consider the report by Claude.
Summary
MaxClockStepSec=limit for the absolute offset accepted from one NTP response;infinity.This implements the policy control requested in #17782. A finite default is intentionally not proposed: systems without a reliable RTC may legitimately require a large initial correction. Administrators that have a trustworthy starting clock can now select an appropriate ceiling.
Testing
Tested from current
mainat765dc9691be793e03b6b13b16bb834c0aa216c9eon a Debian testing ARM64 host:The unit coverage checks the unlimited default, the exact configured boundary, and positive and negative offsets beyond it.
git diff --checkand XML well-formedness checks also pass.Closes #17782.