NAK: [F][PATCH 0/1] rtc: class: support hctosys from modular RTC drivers

[Jesse Sung]

On Thu, Nov 25, 2021 at 9:14 PM Dimitri John Ledkov
<dimitri.ledkov at canonical.com> wrote:
>
> Hi,
>
>
> On Thu, Nov 25, 2021 at 10:09 AM Jesse Sung <jesse.sung at canonical.com> wrote:
> >
> > On Thu, Nov 25, 2021 at 12:00 AM Dimitri John Ledkov
> > <dimitri.ledkov at canonical.com> wrote:
> > >
> > > But doesn't this break assumptions that systemd makes? I.e. it would
> > > sync it's time from the timesync state file, and then loading rtc
> > > module would trump this.
> > >
> > > This change in v5.7 kernel has caused breakage. Does introducing this
> > > in v5.4 kernel going to break UC20 machines which sync a better/secure
> > > time in the initrd; and then will get broken time from the rtc module
> > > because it happens to not be battery backed. Leading to inability to
> > > snap refresh?
> >
> > IMHO a proper solution for this is to have a way to add a list of
> > modules when we use ubuntu-core-initramfs to generate the initrd.
> > Something like /etc/initramfs-tools/modules in the classic image. For
> > example:
> > ubuntu-core-initram --extra-kernel-modules <mod1>,<mod2>,<mod3>
> > So on one hand we don't need to make every RTC driver built-in, and on
> > the other hand hctosys happens before systemd tries to access system
> > time hence it acts like a built-in driver to systemd.
> >
>
> That has not been true for a while now in recent updates of UC20.

Humm... May I know which part has not been true?

> ubuntu-core-initrd starts systemd inside the initrd, it looks up the
> timestamp from /usr/lib/clock-epoch which is updated at every initrd
> creation time and sets that as a system clock, if this timestamp is
> older than the current system time.
>
> Then, snapd-bootstrap runs and reads & validates signed assertions. If
> it finds any valid assertions with a newer timestamp than the current
> time, it again updates the system clock.
>
> Thus all UC20 builds of recent enough kernels (Since around March
> 2021) come up correctly with correct time of MAX ( initrd built time,
> any assertions used by the image). Since implementing this feature we
> have had no bug reports about UC20 machines failing to boot due to
> wrong time, including from people using stock kernel, yet brand new
> models/account-keys/etc.

With all these checks and validations, how would reading the RTC at
the very beginning of init scripts hurt UC20 systems even if RTC time
is invalid? Shouldn't it be corrected by these later checks?

> > >
> > > Can you please explain what is prompting to backport this change into
> > > v5.4 kernels?
> >
> > For systems which don't have Internet access but do have valid RTC
> > time, if the RTC driver is a module, the RTC time will never be used
> > and instead UC20 would start from a fixed time which causes problems
> > in some projects.
> >
>
> This sounds not true since Mar 2021. Given the above.

Given the above, if a system doesn't have network access and has its
RTC driver as a module, it would start its system time from "MAX (
initrd built time, any assertions used by the image)" every time it
boots until it gets a kernel snap update, even if there's valid RTC
time, is that correct?

> Strong NAK on applying this patch, given the core-initrd solutions
> available since Mar 2021, which this patch will undo. Cause it will
> reset time to random one, upon loading RTC module which may go back in
> time to the past prior to initrd time-epoch & snapd assertions used to
> boot.

If a built-in RTC driver is not an issue, why would an RTC driver
loaded as early as possible be one?

> The kernel RTC / hctosys must be fixed to not reset the clock into the
> past; if it has already been adjusted by the userspace. At the moment
> hctosys & userspace are racing each other, with userspace having
> access to more time information than a battery-less RTC module.

Kernel hctosys only happens on CONFIG_RTC_HCTOSYS_DEVICE, so for UC20
systems as long as the specific driver gets loaded early it should be
fine. Also it's not feasible to make every RTC driver built-in just
for using the valid time in RTC.
And the change doesn't seem to hurt Ubuntu desktop and server systems either.

Thanks,
Jesse

>
>
> > Thanks,
> > Jesse
> >
> > >
> > >
> > > --
> > > Regards,
> > >
> > > Dimitri.
> > >
> > > On Wed, Nov 24, 2021 at 11:01 AM Wen-chien Jesse Sung
> > > <jesse.sung at canonical.com> wrote:
> > > >
> > > > BugLink: https://launchpad.net/bugs/1952077
> > > >
> > > > == Impact ==
> > > > Although we have CONFIG_RTC_HCTOSYS enabled and set CONFIG_RTC_HCTOSYS_DEVICE
> > > > to "rtc0", system time would only be set if the RTC driver is built-in. This
> > > > may not be an issue on most amd64 systems because CONFIG_RTC_DRV_CMOS is y
> > > > for amd64, however it is an issue for other architectures since there may not
> > > > be a proper RTC driver enabled as built-in. Also making every RTC driver that
> > > > may be used as built-in is not a good option.
> > > >
> > > > == Fix ==
> > > > The commit is in mainline since v5.7:
> > > >
> > > > == Risk of Regression ==
> > > > It's a clean cherry-pick for v5.4 and looks straight forward so risk should be low.
> > > >
> > > >
> > > > Steve Muckle (1):
> > > >   rtc: class: support hctosys from modular RTC drivers
> > > >
> > > >  drivers/rtc/Kconfig   |  3 --
> > > >  drivers/rtc/Makefile  |  1 -
> > > >  drivers/rtc/class.c   | 61 ++++++++++++++++++++++++++++++++++++++
> > > >  drivers/rtc/hctosys.c | 69 -------------------------------------------
> > > >  4 files changed, 61 insertions(+), 73 deletions(-)
> > > >  delete mode 100644 drivers/rtc/hctosys.c
> > > >
> > > > --
> > > > 2.25.1
> > > >
> > > >
> > > > --
> > > > kernel-team mailing list
> > > > kernel-team at lists.ubuntu.com
> > > > https://lists.ubuntu.com/mailman/listinfo/kernel-team