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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1330674796.2361.36.camel@work-vm>
Date:	Thu, 01 Mar 2012 23:53:16 -0800
From:	John Stultz <john.stultz@...aro.org>
To:	Ingo Molnar <mingo@...e.hu>
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

On Fri, 2012-03-02 at 08:38 +0100, Ingo Molnar wrote:
> * John Stultz <john.stultz@...aro.org> wrote:
> 
> > 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.

So I actually removed the locks out from the timekeeper structure to try
to address this concern.

> 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()?

Just about all of timekeeper state is used and modified in the
update_wall_time. 

> 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.)

So, just to be clear, you want me to push basically everything in the
timekeeper structure, except the lock (which would be re-added), into a
time_state sub-structure?

Sorry for being dense here (its been a long day), but maybe could you
clarify a bit more about the differences you're describing between
time-state and global-state wrt the timekeeper?

thanks
-john

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