linux-kernel - Re: [PATCH 2/5] clocksource: Allow clocksource_mark

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]

Date:   Mon, 23 Apr 2018 18:41:13 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     tglx@...utronix.de
Cc:     linux-kernel@...r.kernel.org, diego.viola@...il.com,
        len.brown@...el.com, rjw@...ysocki.net, rui.zhang@...el.com
Subject: Re: [PATCH 2/5] clocksource: Allow clocksource_mark_unstable() on
 unregisered clocksources

On Mon, Apr 23, 2018 at 06:16:50PM +0200, Peter Zijlstra wrote:
> Because of how we flip between tsc-early and tsc clocksources we might
> need to mark one or both unstable. The current code in
> mark_tsc_unstable() only worked because we registered the tsc
> clocksource once and then never touched it.
> 
> Since we now unregister the tsc-early clocksource, we need to know if
> a clocksource got unregistered and the current cs->mult test doesn't
> work for that. Instead use list_empty(&cs->list) to test for
> registration.
> 
> Furthermore, since clocksource_mark_unstable() needs to place the cs
> on the wd_list, it links the cs->list and cs->wd_list serialization.
> We must not see a clocsource registered (!empty cs->list) but already
> past dequeue_watchdog(). So place {en,de}queue{,_watchdog}() under the
> same lock.
> 
> This then allows us to unconditionally use
> clocksource_mark_unstable(), regardless of the registration state.
> 

I think we also want the below patch, either merged in or on top.
Because __clocksource_change_rating() actually requires holding that
lock now.

I've no idea why it says it cannot do that with the lock held. But
there's more stale comments in there.

I'll actually test it later, need to head out to the Dojo soon.


--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -405,14 +405,13 @@ static int __clocksource_watchdog_kthrea
 {
 	struct clocksource *cs, *tmp;
 	unsigned long flags;
-	LIST_HEAD(unstable);
 	int select = 0;
 
 	spin_lock_irqsave(&watchdog_lock, flags);
 	list_for_each_entry_safe(cs, tmp, &watchdog_list, wd_list) {
 		if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
 			list_del_init(&cs->wd_list);
-			list_add(&cs->wd_list, &unstable);
+			__clocksource_change_rating(cs, 0);
 			select = 1;
 		}
 		if (cs->flags & CLOCK_SOURCE_RESELECT) {
@@ -424,11 +423,6 @@ static int __clocksource_watchdog_kthrea
 	clocksource_stop_watchdog();
 	spin_unlock_irqrestore(&watchdog_lock, flags);
 
-	/* Needs to be done outside of watchdog lock */
-	list_for_each_entry_safe(cs, tmp, &unstable, wd_list) {
-		list_del_init(&cs->wd_list);
-		__clocksource_change_rating(cs, 0);
-	}
 	return select;
 }