[PATCH] fix gettimeofday vs. update_gtod race

[PATCH] fix gettimeofday vs. update_gtod race

[Nathan Lynch]

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 <ntl@pobox.com>

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

Re: [PATCH] fix gettimeofday vs. update_gtod race

[Benjamin Herrenschmidt]

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.

>  	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
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] fix gettimeofday vs. update_gtod race

[Nathan Lynch]

Benjamin Herrenschmidt wrote:
> 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

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.

Re: [PATCH] fix gettimeofday vs. update_gtod race

[Benjamin Herrenschmidt]

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.

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

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

Re: [PATCH] fix gettimeofday vs. update_gtod race

[Nathan Lynch]

Benjamin Herrenschmidt wrote:
> 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.

I see, thanks.

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

Re: [PATCH] fix gettimeofday vs. update_gtod race

[Nathan Lynch]

Nathan Lynch wrote:
> Benjamin Herrenschmidt wrote:
> > 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.
> 
> 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."

Re: [PATCH] fix gettimeofday vs. update_gtod race

[Benjamin Herrenschmidt]

On Mon, 2006-08-21 at 16:25 -0500, Nathan Lynch wrote:
> Nathan Lynch wrote:
> > Benjamin Herrenschmidt wrote:
> > > 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.
> > 
> > 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.