fix gettimeofday vs. update_gtod race

PATCH In review Nathan Lynch 2 participants 7 messages Aug 11, 2006 – Aug 21, 2006 PowerPC

Share & tools

Nathan Lynch

author

2006-08-11 20:41 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

arch/powerpc/kernel/time.c +8 -3

index 774c0a3..6b690df 100644
@@ -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);

Benjamin Herrenschmidt

2006-08-16 23:49 UTC #1 lore raw

arch/powerpc/kernel/time.c static inline void __do_gettimeofday(str · line 439

Quoted 5 lines · Nathan Lynch

arch/powerpc/kernel/time.c +5 -0

+	/* 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.

Quoted 17 lines

arch/powerpc/kernel/time.c +1 -1

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

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Nathan Lynch

author

2006-08-17 00:18 UTC #2 lore raw

Quoted 9 lines · Benjamin Herrenschmidt

arch/powerpc/kernel/time.c static inline void __do_gettimeofday(str · line 439
Quoted 5 lines · Nathan Lynch
arch/powerpc/kernel/time.c +5 -0
+	/* 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?

Quoted 1 line

and the race not completely closed imho...

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

Quoted 4 lines

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.

Benjamin Herrenschmidt

2006-08-17 00:28 UTC #3 lore raw

Quoted 3 lines · Nathan Lynch

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.

Quoted 1 line

Surely at least a compiler barrier is needed?

Yeah.

Quoted 4 lines

Quoted 1 line
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 7 lines

Quoted 4 lines
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.

Nathan Lynch

author

2006-08-17 20:47 UTC #4 lore raw

Quoted 7 lines · Benjamin Herrenschmidt

Quoted 3 lines · Nathan Lynch
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 9 lines

Quoted 7 lines
Quoted 4 lines
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

author

2006-08-21 21:26 UTC #5 lore raw

Quoted 10 lines · Nathan Lynch

Quoted 7 lines · Benjamin Herrenschmidt
Quoted 3 lines · Nathan Lynch
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."

Benjamin Herrenschmidt

2006-08-21 21:43 UTC #6 lore raw

Quoted 8 lines · Nathan Lynch

Quoted 7 lines · Nathan Lynch
Quoted 6 lines · Benjamin Herrenschmidt
Quoted 3 lines · Nathan Lynch
No? I didn't find anything about mftb having synchronizing behavior. How should we ensure that temp_varp is assigned before reading the timebase?

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
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.