[RFC][PATCH] Use proper casting with signed timespec.tv_nsec values

[RFC][PATCH] Use proper casting with signed timespec.tv_nsec values

[john stultz]

All,
	I recently ran into a bug with an older kernel where xtime's tv_nsec
field had accumulated more then 2 seconds worth of time. The timespec's
tv_nsec is a signed long, however gettimeofday() treats it as an
unsigned long. Thus when the failure occured, very strange and difficult
to debug time problems occurred.

The main cause of the problem I was seeing is already fixed in mainline,
however just to be safe, I figured the following patch would be wise.

I only audited i386 and x86_64, however other arches probably could have
similar signed problems as well.

Please let me know if you have any further comments or feedback.

thanks
-john

linux-2.6.13_signed-tv_nsec_A0.patch
====================================
diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
--- a/arch/i386/kernel/time.c
+++ b/arch/i386/kernel/time.c
@@ -156,7 +156,7 @@ void do_gettimeofday(struct timeval *tv)
 			usec += lost * (USEC_PER_SEC / HZ);
 
 		sec = xtime.tv_sec;
-		usec += (xtime.tv_nsec / 1000);
+		usec += (unsigned long)xtime.tv_nsec / 1000;
 	} while (read_seqretry(&xtime_lock, seq));
 
 	while (usec >= 1000000) {
diff --git a/arch/x86_64/kernel/time.c b/arch/x86_64/kernel/time.c
--- a/arch/x86_64/kernel/time.c
+++ b/arch/x86_64/kernel/time.c
@@ -128,7 +128,7 @@ void do_gettimeofday(struct timeval *tv)
 		seq = read_seqbegin(&xtime_lock);
 
 		sec = xtime.tv_sec;
-		usec = xtime.tv_nsec / 1000;
+		usec = (unsigned long)xtime.tv_nsec / 1000;
 
 		/* i386 does some correction here to keep the clock 
 		   monotonous even when ntpd is fixing drift.
diff --git a/kernel/timer.c b/kernel/timer.c
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -824,7 +824,7 @@ static void update_wall_time(unsigned lo
 	do {
 		ticks--;
 		update_wall_time_one_tick();
-		if (xtime.tv_nsec >= 1000000000) {
+		if ((unsigned long)xtime.tv_nsec >= 1000000000) {
 			xtime.tv_nsec -= 1000000000;
 			xtime.tv_sec++;
 			second_overflow();

Re: [RFC][PATCH] Use proper casting with signed timespec.tv_nsec values

[Eric Dumazet]

john stultz a écrit :
> All,
> 	I recently ran into a bug with an older kernel where xtime's tv_nsec
> field had accumulated more then 2 seconds worth of time. The timespec's
> tv_nsec is a signed long, however gettimeofday() treats it as an
> unsigned long. Thus when the failure occured, very strange and difficult
> to debug time problems occurred.
> 
> The main cause of the problem I was seeing is already fixed in mainline,
> however just to be safe, I figured the following patch would be wise.
> 
> I only audited i386 and x86_64, however other arches probably could have
> similar signed problems as well.
> 
> Please let me know if you have any further comments or feedback.

What happens on i386 when/if more than 4 seconds accumulate ?
That should happens too.
Maybe the real fix is elsewhere ?

> 
> thanks
> -john
> 
> linux-2.6.13_signed-tv_nsec_A0.patch
> ====================================
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -824,7 +824,7 @@ static void update_wall_time(unsigned lo
>  	do {
>  		ticks--;
>  		update_wall_time_one_tick();
> -		if (xtime.tv_nsec >= 1000000000) {
> +		if ((unsigned long)xtime.tv_nsec >= 1000000000) {
>  			xtime.tv_nsec -= 1000000000;
>  			xtime.tv_sec++;
>  			second_overflow();
> 

maybe here a :

while ((unsigned long)xtime.tv_nsec >= 1000000000) {
	xtime.tv_nsec -= 1000000000;
	xtime.tv_sec++;
	second_overflow();
	...
	}


Eric

Re: [RFC][PATCH] Use proper casting with signed timespec.tv_nsec values

[john stultz]

On Thu, 2005-09-01 at 23:37 +0200, Eric Dumazet wrote:
> john stultz a écrit :
> > All,
> > 	I recently ran into a bug with an older kernel where xtime's tv_nsec
> > field had accumulated more then 2 seconds worth of time. The timespec's
> > tv_nsec is a signed long, however gettimeofday() treats it as an
> > unsigned long. Thus when the failure occured, very strange and difficult
> > to debug time problems occurred.
> > 
> > The main cause of the problem I was seeing is already fixed in mainline,
> > however just to be safe, I figured the following patch would be wise.
> > 
> > I only audited i386 and x86_64, however other arches probably could have
> > similar signed problems as well.
> > 
> > Please let me know if you have any further comments or feedback.
> 
> What happens on i386 when/if more than 4 seconds accumulate ?

Well, then we overflow.

> That should happens too.
> Maybe the real fix is elsewhere ?

We just don't have much more then 4 seconds worth of bits there. So I
don't know what your suggesting. Granted, my patch is a bit paranoid, it
tries to make the casts more explicit so when something goes wrong, it
makes more sense (then, say, strange 70minute jumps in time caused by
the signed divide being implicitly cast unsigned).

> > 
> > linux-2.6.13_signed-tv_nsec_A0.patch
> > ====================================
> > --- a/kernel/timer.c
> > +++ b/kernel/timer.c
> > @@ -824,7 +824,7 @@ static void update_wall_time(unsigned lo
> >  	do {
> >  		ticks--;
> >  		update_wall_time_one_tick();
> > -		if (xtime.tv_nsec >= 1000000000) {
> > +		if ((unsigned long)xtime.tv_nsec >= 1000000000) {
> >  			xtime.tv_nsec -= 1000000000;
> >  			xtime.tv_sec++;
> >  			second_overflow();
> > 
> 
> maybe here a :
> 
> while ((unsigned long)xtime.tv_nsec >= 1000000000) {
> 	xtime.tv_nsec -= 1000000000;
> 	xtime.tv_sec++;
> 	second_overflow();
> 	...
> 	}

That would only be necessary if update_wall_time_one_tick() could
increment tv_nsec by more then 1 billion. That seems to be a touch
over-cautious, considering we're already in a while loop. 

thanks
-john

Re: [RFC][PATCH] Use proper casting with signed timespec.tv_nsec values

[George Anzinger]

john stultz wrote:
> All,
> 	I recently ran into a bug with an older kernel where xtime's tv_nsec
> field had accumulated more then 2 seconds worth of time. The timespec's
> tv_nsec is a signed long, however gettimeofday() treats it as an
> unsigned long. Thus when the failure occured, very strange and difficult
> to debug time problems occurred.
> 
> The main cause of the problem I was seeing is already fixed in mainline,
> however just to be safe, I figured the following patch would be wise.
> 
> I only audited i386 and x86_64, however other arches probably could have
> similar signed problems as well.
> 
> Please let me know if you have any further comments or feedback.

John,

There is a problem in the way this code handles the conversion to usec. 
  There is a conversion here and also in the get_offset code.  If the 
nanoseconds are carrier until after the addition of the two about 25% of 
the time you will end up with an additional usec in time.  I strongly 
suggest changing to convert to usec after the addition of xtime and 
get_offset time to avoid this.  If the "correct" thing is done in 
clock_gettime() (i.e. get_offset is in nanoseconds) this actually turns 
up as a back step in time WRT gettimeofday and clock_gettime().

George
-- 
> 
> thanks
> -john
> 
> linux-2.6.13_signed-tv_nsec_A0.patch
> ====================================
> diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
> --- a/arch/i386/kernel/time.c
> +++ b/arch/i386/kernel/time.c
> @@ -156,7 +156,7 @@ void do_gettimeofday(struct timeval *tv)
>  			usec += lost * (USEC_PER_SEC / HZ);
>  
>  		sec = xtime.tv_sec;
> -		usec += (xtime.tv_nsec / 1000);
> +		usec += (unsigned long)xtime.tv_nsec / 1000;
>  	} while (read_seqretry(&xtime_lock, seq));
>  
>  	while (usec >= 1000000) {
> diff --git a/arch/x86_64/kernel/time.c b/arch/x86_64/kernel/time.c
> --- a/arch/x86_64/kernel/time.c
> +++ b/arch/x86_64/kernel/time.c
> @@ -128,7 +128,7 @@ void do_gettimeofday(struct timeval *tv)
>  		seq = read_seqbegin(&xtime_lock);
>  
>  		sec = xtime.tv_sec;
> -		usec = xtime.tv_nsec / 1000;
> +		usec = (unsigned long)xtime.tv_nsec / 1000;
>  
>  		/* i386 does some correction here to keep the clock 
>  		   monotonous even when ntpd is fixing drift.
> diff --git a/kernel/timer.c b/kernel/timer.c
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -824,7 +824,7 @@ static void update_wall_time(unsigned lo
>  	do {
>  		ticks--;
>  		update_wall_time_one_tick();
> -		if (xtime.tv_nsec >= 1000000000) {
> +		if ((unsigned long)xtime.tv_nsec >= 1000000000) {
>  			xtime.tv_nsec -= 1000000000;
>  			xtime.tv_sec++;
>  			second_overflow();
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
George Anzinger   george@mvista.com
HRT (High-res-timers):  http://sourceforge.net/projects/high-res-timers/

Re: [RFC][PATCH] Use proper casting with signed timespec.tv_nsec values

[Christoph Lameter]

On Thu, 1 Sep 2005, john stultz wrote:

> 	I recently ran into a bug with an older kernel where xtime's tv_nsec
> field had accumulated more then 2 seconds worth of time. The timespec's
> tv_nsec is a signed long, however gettimeofday() treats it as an
> unsigned long. Thus when the failure occured, very strange and difficult
> to debug time problems occurred.

How can that happen? I think the source of the problem needs to be fixed. 
The fix is only going decrease the likelyhood of the problem occurring.

We may need special measures if the system was frozen for some 
reason for longer than a certain time period.

Re: [RFC][PATCH] Use proper casting with signed timespec.tv_nsec values

[john stultz]

On Tue, 2005-09-06 at 10:33 -0700, Christoph Lameter wrote:
> On Thu, 1 Sep 2005, john stultz wrote:
> 
> > 	I recently ran into a bug with an older kernel where xtime's tv_nsec
> > field had accumulated more then 2 seconds worth of time. The timespec's
> > tv_nsec is a signed long, however gettimeofday() treats it as an
> > unsigned long. Thus when the failure occured, very strange and difficult
> > to debug time problems occurred.
> 
> How can that happen? I think the source of the problem needs to be fixed. 
> The fix is only going decrease the likelyhood of the problem occurring.

Really it shouldn't, I'm just adding the extra casting to be more
explicit to if at some point in the future a bug does creep up, it will
be easier to understand. The code is assuming we're unsigned, so why not
make that clear to the compiler?

Granted, its not really all that critical and it is a bit paranoid. I
just figured I'd float the patch to see if folks thought it would be a
good idea. 

thanks
-john