linux-kernel - Re: [PATCH 4/9] time: Update timekeeper structure using a local shadow

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20120302073848.GB32401@elte.hu>
Date:	Fri, 2 Mar 2012 08:38:48 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	John Stultz <john.stultz@...aro.org>
Cc:	lkml <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH 4/9] time: Update timekeeper structure using a local
 shadow

* John Stultz <john.stultz@...aro.org> wrote:

> Uses a local shadow structure to update the timekeeper. This
> will allow for reduced timekeeper.rlock hold time.
> 
> CC: Ingo Molnar <mingo@...e.hu>
> CC: Thomas Gleixner <tglx@...utronix.de>
> CC: Eric Dumazet <eric.dumazet@...il.com>
> CC: Richard Cochran <richardcochran@...il.com>
> Signed-off-by: John Stultz <john.stultz@...aro.org>
> ---
>  kernel/time/timekeeping.c |   43 +++++++++++++++++++++++--------------------
>  1 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index f9ee96c..09460c1 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -74,6 +74,7 @@ struct timekeeper {
>  };
>  
>  static struct timekeeper timekeeper;
> +static struct timekeeper shadow_tk;

Sigh.

As I said it in the first round of review, it's fundamentally 
wrong to copy live fields like locks or the clocksource pointer 
around.

It's doubly wrong to do it in a global variable that no-one else 
but the copying function (update_wall_time()) is supposed to 
access.

There are over a dozen fields in 'struct timekeeper' - exactly 
which ones of them are used on this private copy, as 
update_wall_time() does the cycle accumulation and calls down 
into timkeeping_adjust()?

The right solution would be to separate timekeeping time state 
from global state:

struct timekeeper {
	spinlock_t		lock;

	struct time_state	time_state;
};

And then standardize the time calculation code on passing around 
not 'struct timekeeper *' but 'struct time_state *' ! Then you 
can have a local shadow copy of the global state:

	struct time_state time_state_copy;

and copy it from the global one and then pass it down to 
calculation functions.

This also gives the freedom to add other global state fields 
beyond the lock. (Right now the lock appears to be the only 
global state field - there might be more.)

Thanks,

	Ingo
--
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/