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: <87a6uwdnfn.fsf@nanos.tec.linutronix.de>
Date:   Wed, 02 Dec 2020 20:21:00 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     Miroslav Lichvar <mlichvar@...hat.com>,
        linux-kernel@...r.kernel.org, John Stultz <john.stultz@...aro.org>,
        Prarit Bhargava <prarit@...hat.com>
Subject: Re: [PATCH] rtc: adapt allowed RTC update error

On Wed, Dec 02 2020 at 12:27, Jason Gunthorpe wrote:
> On Wed, Dec 02, 2020 at 02:44:53PM +0100, Thomas Gleixner wrote:
>>  	if (IS_ENABLED(CONFIG_GENERIC_CMOS_UPDATE) ||
>>  	    IS_ENABLED(CONFIG_RTC_SYSTOHC))
>> -		queue_delayed_work(system_power_efficient_wq, &sync_work, 0);
>> +		queue_work(system_power_efficient_wq, &sync_work);
>
> As Miroslav noted, probably the right thing to do here is to reset the
> hrtimer and remove the sync_work? I think this code was to expedite an
> RTC sync when NTP fixes the clock on boot.

This has two purposes:

     1) Initiating the update on boot once ntp is synced.

     2) Reinitiating the sync after ntp lost sync and the work did not
        reschedule itself because it observed !ntp_synced().

In both cases it's highly unlikely that the write actually happens when
the work is queued because do_adjtimex() would have to be exactly around
the valid update window. So it will not write immediately. It will run
through at least one retry.

I don't think the timer should be canceled if the ntp_synced() state did
not change. Otherwise every do_adtimex() call will cancel/restart
it, which does not make sense. Lemme stare at it some more.

> IIRC this was made somewhat messy due to the dual path with rtclib and
> old cmos.

:)

> It would be very nice to kill the cmos path, things are better
> now.. But PPC still has a long way to go:
>
> arch/powerpc/platforms/52xx/efika.c:    .set_rtc_time           = rtas_set_rtc_time,
> arch/powerpc/platforms/8xx/mpc86xads_setup.c:   .set_rtc_time           = mpc8xx_set_rtc_time,
> arch/powerpc/platforms/8xx/tqm8xx_setup.c:      .set_rtc_time           = mpc8xx_set_rtc_time,
> arch/powerpc/platforms/cell/setup.c:    .set_rtc_time           = rtas_set_rtc_time,
> arch/powerpc/platforms/chrp/setup.c:            ppc_md.set_rtc_time     = rtas_set_rtc_time;
> arch/powerpc/platforms/chrp/setup.c:    .set_rtc_time           = chrp_set_rtc_time,
> arch/powerpc/platforms/maple/setup.c:   .set_rtc_time           = maple_set_rtc_time,
> arch/powerpc/platforms/powermac/setup.c:        .set_rtc_time           = pmac_set_rtc_time,
> arch/powerpc/platforms/pseries/setup.c: .set_rtc_time           = rtas_set_rtc_time,
>
> Also x86 needs a touch, it already has RTC lib, no idea why it also
> provides this old path too

Because nobody had the stomach and/or cycles to touch it :)

> I wonder if the cmos path could be killed off under the dead HW
> principle?

Unfortunately that code path is not that dead on x86. You need to fix
all the (ab)users first. :)

Thanks,

        tglx