[PATCH] fix gettimeofday vs. update_gtod race

Userspace can (very, very) occasionally get bogus time values due to
a tiny race between powerpc's do_gettimeofday and timer interrupt:

1. do_gettimeofday does get_tb()

2. decrementer exception on boot cpu which runs timer_recalc_offset,
   which also samples the timebase and updates the do_gtod structure
   with a greater timebase value.

3. do_gettimeofday calls __do_gettimeofday, which leads to the
   negative result from tb_val - temp_varp->tb_orig_stamp.

The fix is to ensure that do_gettimeofday samples the timebase only
after loading do_gtod.varp.


Signed-off-by: Nathan Lynch <redacted>

diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 774c0a3..6b690df 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c

@@ -417,7 +417,7 @@ static __inline__ void timer_check_rtc(v
 /*
  * This version of gettimeofday has microsecond resolution.
  */
-static inline void __do_gettimeofday(struct timeval *tv, u64 tb_val)
+static inline void __do_gettimeofday(struct timeval *tv)
 {
 	unsigned long sec, usec;
 	u64 tb_ticks, xsec;

@@ -431,7 +431,12 @@ static inline void __do_gettimeofday(str
 	 * without a divide (and in fact, without a multiply)
 	 */
 	temp_varp = do_gtod.varp;
-	tb_ticks = tb_val - temp_varp->tb_orig_stamp;
+
+	/* Sampling the time base must be done after loading
+	 * do_gtod.varp in order to avoid racing with update_gtod.
+	 */
+	rmb();
+	tb_ticks = get_tb() - temp_varp->tb_orig_stamp;
 	temp_tb_to_xs = temp_varp->tb_to_xs;
 	temp_stamp_xsec = temp_varp->stamp_xsec;
 	xsec = temp_stamp_xsec + mulhdu(tb_ticks, temp_tb_to_xs);

@@ -464,7 +469,7 @@ void do_gettimeofday(struct timeval *tv)
 		tv->tv_usec = usec;
 		return;
 	}
-	__do_gettimeofday(tv, get_tb());
+	__do_gettimeofday(tv);
 }
 
 EXPORT_SYMBOL(do_gettimeofday);

On Fri, 2006-08-11 at 15:41 -0500, Nathan Lynch wrote:

+	/* Sampling the time base must be done after loading
+	 * do_gtod.varp in order to avoid racing with update_gtod.
+	 */
+	rmb();
+	tb_ticks = get_tb() - temp_varp->tb_orig_stamp;

The barrier isn't necessary and the race not completely closed imho... I
need to think about it a bit more closely but what about instead just
check if tb_ticks goes negative, and if yes, just do get_tb() again ?
That might be faster than having a sync in there and should still be
correct.

Benjamin Herrenschmidt wrote:

On Fri, 2006-08-11 at 15:41 -0500, Nathan Lynch wrote:

quoted

+	/* Sampling the time base must be done after loading
+	 * do_gtod.varp in order to avoid racing with update_gtod.
+	 */
+	rmb();
+	tb_ticks = get_tb() - temp_varp->tb_orig_stamp;

The barrier isn't necessary

No?  I didn't find anything about mftb having synchronizing
behavior.  How should we ensure that temp_varp is assigned before
reading the timebase?

Surely at least a compiler barrier is needed?

and the race not completely closed imho... 

How so?  I could've missed something, but I've hammered the patch
pretty hard, fwiw.

I need to think about it a bit more closely but what about instead
just check if tb_ticks goes negative, and if yes, just do get_tb()
again ?  That might be faster than having a sync in there and should
still be correct.

I did try something like that but found that a loop (i.e. multiple
get_tb's to "catch up") was necessary.

On Wed, 2006-08-16 at 19:18 -0500, Nathan Lynch wrote:

No?  I didn't find anything about mftb having synchronizing
behavior.  How should we ensure that temp_varp is assigned before
reading the timebase?

I sync an isync would be enough.

Surely at least a compiler barrier is needed?

Yeah.

quoted

and the race not completely closed imho... 

How so?  I could've missed something, but I've hammered the patch
pretty hard, fwiw.

Nah you are right, but you may be using a too big hammer

quoted

I need to think about it a bit more closely but what about instead
just check if tb_ticks goes negative, and if yes, just do get_tb()
again ?  That might be faster than having a sync in there and should
still be correct.

I did try something like that but found that a loop (i.e. multiple
get_tb's to "catch up") was necessary.

Hrm... even with an isync ?

Ben.

Benjamin Herrenschmidt wrote:

On Wed, 2006-08-16 at 19:18 -0500, Nathan Lynch wrote:

quoted

No?  I didn't find anything about mftb having synchronizing
behavior.  How should we ensure that temp_varp is assigned before
reading the timebase?

I sync an isync would be enough.

I see, thanks.

quoted

quoted

I need to think about it a bit more closely but what about instead
just check if tb_ticks goes negative, and if yes, just do get_tb()
again ?  That might be faster than having a sync in there and should
still be correct.

I did try something like that but found that a loop (i.e. multiple
get_tb's to "catch up") was necessary.

Hrm... even with an isync ?

No, sorry, I was confusing this with a different bug (cpu
hotplug-related, separate patch for that forthcoming).

Nathan Lynch wrote:

Benjamin Herrenschmidt wrote:

quoted

On Wed, 2006-08-16 at 19:18 -0500, Nathan Lynch wrote:

quoted

No?  I didn't find anything about mftb having synchronizing
behavior.  How should we ensure that temp_varp is assigned before
reading the timebase?

I sync an isync would be enough.

I see, thanks.

Actually, after checking Book 2 and discussing with some other folks
I'm not so sure -- isync "may complete before storage accesses
associated with instructions preceding the isync instruction have been
performed."

On Mon, 2006-08-21 at 16:25 -0500, Nathan Lynch wrote:

Nathan Lynch wrote:

quoted

Benjamin Herrenschmidt wrote:

quoted

On Wed, 2006-08-16 at 19:18 -0500, Nathan Lynch wrote:

quoted

No?  I didn't find anything about mftb having synchronizing
behavior.  How should we ensure that temp_varp is assigned before
reading the timebase?

I sync an isync would be enough.

I see, thanks.

Actually, after checking Book 2 and discussing with some other folks
I'm not so sure -- isync "may complete before storage accesses
associated with instructions preceding the isync instruction have been
performed."

Of sure, I was thinking about isync preventing mftb from being executed
and we can have a proper data dependency. Anyway, that's not necessary,
I've looked at the code and we no longer need to pass the tb value in
(it's historical). Thus we can just move the mftb in the protected area
and maybe with a twi/isync pair make sure we got the gtod pointer before
we do the mftb

Ben.