| 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
| ||
|
Message-ID: <473CACA9.2090702@reed.com>
Date: Thu, 15 Nov 2007 15:31:37 -0500
From: "David P. Reed" <dpreed@...d.com>
To: Thomas Gleixner <tglx@...utronix.de>
CC: linux-kernel@...r.kernel.org,
Allessandro Zummo <a.zummo@...ertech.it>,
Matt Mackall <mpm@...enic.com>
Subject: Re: [PATCH] x86: on x86_64, correct reading of PC RTC when update
in progress in time_64.c
There are a couple of things I don't understand on this one. And I
presume you thought the other two bug fixing patches I sent before this
were OK to go, since on my system
Thomas Gleixner wrote:
> Still whitespace wreckage in your patches. I guess the kernel tree you
> made your patches against is already white space wrecked.
>
> I fixed that up manually, but please be more careful about that next
> time.
Um ... I fixed the whitespaces I detected from the first round with
checkpatch.pl. Then for good measure
I ran checkpatch.pl on the patches, then pasted the files directly into
the emails. No problems detected.
And I also just tried checkpatch.pl on the "sent" folder copy. No
problems detected there.
Where was the whitespace? Was it in the patches? Would you mind showing
me the output so I can do a better job in the future?
>
>> Correct potentially unstable PC RTC time register reading in time_64.c
>>
>> Stop the use of an incorrect technique for reading the standard PC RTC
>> timer, which is documented to "disconnect" time registers from the bus
>> while updates are in progress. The use of UIP flag while interrupts
>> are disabled to protect a 244 microsecond window is one of the
>> Motorola spec sheet's documented ways to read the RTC time registers
>> reliably.
>>
>> The patch updates the misleading comments and also minimizes the amount of
>> time that the kernel disables interrupts during the reading.
>>
>
> While I think that the UIP change is correct and a must have, the
> locking change is not really useful. read_persistent_clock is called
> from exactly three places:
>
What locking change? I didn't change how locking works in
read_persistent_clock at all.
I did introduce cpu_relax() because if anyone else ever calls from a hot
path, that would be good practice and its' one line.
> Right after boot, right before suspend and right after resume. None of
> those places is a hot path, where we really care about the interrupt
> enable/disable. IIRC, this is even called with interrupts disabled
> most of the time, so no real gain here.
>
> Another reason not to do the locking change is the paravirt stuff
> which is coming for 64bit. I looked into the existing 32bit code and
> doing the same lock thing would introduce a real nasty hackery, which
> is definitely not worth the trouble.
>
I presume time_64.c and time_32.c will be unified at some point,
discarding time_64.c. There's no arch-specific reason to be separate.
The current time_32.c depends on a different nmi path (that does some
weird stuff saving and restoring the CMOS index register!), and I didn't
dare usurp your long-term plan to unify architectures. But a simple
cleanup here makes sense, lest someone copy the bad technique as if it
were good.
> Thanks,
>
> tglx
>
>
>> Signed-off-by: David P. Reed <dpreed@...d.com>
>> ---
>> Index: linux-2.6/arch/x86/kernel/time_64.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/kernel/time_64.c
>> +++ linux-2.6/arch/x86/kernel/time_64.c
>> @@ -160,22 +160,30 @@ unsigned long read_persistent_clock(void
>> unsigned long flags;
>> unsigned century = 0;
>>
>> - spin_lock_irqsave(&rtc_lock, flags);
>> + retry: spin_lock_irqsave(&rtc_lock, flags);
>> + /* if UIP is clear, then we have >= 244 microseconds before RTC
>> + * registers will be updated. Spec sheet says that this is the
>> + * reliable way to read RTC - registers invalid (off bus) during
>> update
>> + */
>> + if ((CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP)) {
>> + spin_unlock_irqrestore(&rtc_lock, flags);
>> + cpu_relax();
>> + goto retry;
>> + }
>>
>> - do {
>> - sec = CMOS_READ(RTC_SECONDS);
>> - min = CMOS_READ(RTC_MINUTES);
>> - hour = CMOS_READ(RTC_HOURS);
>> - day = CMOS_READ(RTC_DAY_OF_MONTH);
>> - mon = CMOS_READ(RTC_MONTH);
>> - year = CMOS_READ(RTC_YEAR);
>> + /* now read all RTC registers while stable with interrupts disabled */
>> +
>> + sec = CMOS_READ(RTC_SECONDS);
>> + min = CMOS_READ(RTC_MINUTES);
>> + hour = CMOS_READ(RTC_HOURS);
>> + day = CMOS_READ(RTC_DAY_OF_MONTH);
>> + mon = CMOS_READ(RTC_MONTH);
>> + year = CMOS_READ(RTC_YEAR);
>> #ifdef CONFIG_ACPI
>> - if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
>> - acpi_gbl_FADT.century)
>> - century = CMOS_READ(acpi_gbl_FADT.century);
>> + if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
>> + acpi_gbl_FADT.century)
>> + century = CMOS_READ(acpi_gbl_FADT.century);
>> #endif
>> - } while (sec != CMOS_READ(RTC_SECONDS));
>> -
>> spin_unlock_irqrestore(&rtc_lock, flags);
>>
>> /*
>>
>>
>>
>
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists