linux-kernel - Re: [PATCH] rtc: adapt allowed RTC update error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201203172919.GC7535@piout.net>
Date:   Thu, 3 Dec 2020 18:29:19 +0100
From:   Alexandre Belloni <alexandre.belloni@...tlin.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Jason Gunthorpe <jgg@...pe.ca>,
        Miroslav Lichvar <mlichvar@...hat.com>,
        linux-kernel@...r.kernel.org, John Stultz <john.stultz@...aro.org>,
        Prarit Bhargava <prarit@...hat.com>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        linux-rtc@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] rtc: adapt allowed RTC update error

On 03/12/2020 16:39:21+0100, Thomas Gleixner wrote:
> Alexandre,
> 
> On Thu, Dec 03 2020 at 03:10, Alexandre Belloni wrote:
> > On 03/12/2020 02:14:12+0100, Thomas Gleixner wrote:
> >> That said, can somebody answer the one million dollar question which
> >> problem is solved by all of this magic nonsense?
> >> 
> > The goal was to remove the 500ms offset for all the RTCs but the
> > MC146818 because there are RTC that will reset properly their counter
> > when setting the time, meaning they can be set very precisely.
> 
> The MC setting is halfways precise. The write resets the divider chain
> and when the reset is removed then the next UIP will happen after the
> magic 0.5 seconds. So yes, writing it 500ms _before_ the next second is
> halfways correct assumed that there is no interference between
> ktime_get_real() and the actual write which is a silly assumption as the
> code is fully preemptible.
> 
> > IIRC, used in conjunction with rtc_hctosys which also adds
> > inconditionnaly 500ms this can ends up with the system time
> > being one second away from the wall clock time which NTP will take quite
> > some time to remove.
> 
> The rtc_cmos() driver has a fun comment in cmos_set_time()....
> 
> The logic in sync_cmos_clock() and rtc_set_ntp_time() is different as I
> pointed out: sync_cmos_clock() hands -500ms to rtc_tv_nsec_ok() and
> rtc_set_ntp_time() uses +500ms, IOW exactly ONE second difference in
> behaviour.
> 
> > Coincidentally, I was going to revert those patches for v5.11.
> 
> Which will break the rtc_cmos() driver in a different way. We should
> really fix that proper and just have the 500ms offset for rtc_cmos,
> aka. MC146818. When other drivers want a different offset, then they
> still can do so.
> 

My point was to get back to the previous situation where only
rtc_cmos was supposed to work properly by removing rtc_tv_nsec_ok and
set_offset_nsec.

> The direct /dev/rtc settime ioctl is not using that logic anyway. Thats
> the business of the user space application to get that straight which is
> scheduling lottery as well.

I still don't see how userspace is worse than systohc in that regard and
why we need to do that in the kernel. Especially since hctosys is doing
a very bad job trying to read the time from the RTC. You may as well not
bother with the 500ms and just set the time to the current or next
second.

And what about the non configurable 659 period, isn't that policy that
should be left to userspace configuration?

I'm still convinced that set_offset_nsec is not needed to set the time
accurately and I still want to remove it. Also, this may be a good time
to move systohc.c to kernel/time/ntp.c as this is definitively some NTP
specific code being an RTC consumer, very much like alarmtimer.c

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com