[PATCH] fix potential jiffies overflow

[PATCH] fix potential jiffies overflow

[Atsushi Nemoto]

I found i386 timer_resume is updating jiffies, not jiffies_64.  It
looks there is a potential overflow problem.  Is this a correct fix?

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
index a14d594..e4ed172 100644
--- a/arch/i386/kernel/time.c
+++ b/arch/i386/kernel/time.c
@@ -413,7 +413,7 @@ static int timer_resume(struct sys_devic
 	xtime.tv_sec = sec;
 	xtime.tv_nsec = 0;
 	write_sequnlock_irqrestore(&xtime_lock, flags);
-	jiffies += sleep_length;
+	jiffies_64 += sleep_length;
 	wall_jiffies += sleep_length;
 	if (last_timer->resume)
 		last_timer->resume();

Re: [PATCH] fix potential jiffies overflow

[Ram Gupta]

On 3/2/06, Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> I found i386 timer_resume is updating jiffies, not jiffies_64.  It
> looks there is a potential overflow problem.  Is this a correct fix?
>
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
>
> diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
> index a14d594..e4ed172 100644
> --- a/arch/i386/kernel/time.c
> +++ b/arch/i386/kernel/time.c
> @@ -413,7 +413,7 @@ static int timer_resume(struct sys_devic
>         xtime.tv_sec = sec;
>         xtime.tv_nsec = 0;
>         write_sequnlock_irqrestore(&xtime_lock, flags);
> -       jiffies += sleep_length;
> +       jiffies_64 += sleep_length;
>         wall_jiffies += sleep_length;
>         if (last_timer->resume)
>                 last_timer->resume();


The 64-bit jiffies value is not atomic. You need to hold xtime_lock to read it.

Re: [PATCH] fix potential jiffies overflow

[Atsushi Nemoto]

>>>>> On Thu, 2 Mar 2006 10:43:12 -0600, "Ram Gupta" <ram.gupta5@gmail.com> said:
>> I found i386 timer_resume is updating jiffies, not jiffies_64.  It
>> looks there is a potential overflow problem.  Is this a correct
>> fix?

ram> The 64-bit jiffies value is not atomic. You need to hold
ram> xtime_lock to read it.

OK, and I guess wall_jiffies also needs xtime_lock.


I found i386 timer_resume is updating jiffies, not jiffies_64.  It
looks there is a potential overflow problem.  And jiffies_64 and
wall_jiffies should be protected by xtime_lock.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
index a14d594..9d30747 100644
--- a/arch/i386/kernel/time.c
+++ b/arch/i386/kernel/time.c
@@ -412,9 +412,9 @@ static int timer_resume(struct sys_devic
 	write_seqlock_irqsave(&xtime_lock, flags);
 	xtime.tv_sec = sec;
 	xtime.tv_nsec = 0;
-	write_sequnlock_irqrestore(&xtime_lock, flags);
-	jiffies += sleep_length;
+	jiffies_64 += sleep_length;
 	wall_jiffies += sleep_length;
+	write_sequnlock_irqrestore(&xtime_lock, flags);
 	if (last_timer->resume)
 		last_timer->resume();
 	cur_timer = last_timer;

Re: [PATCH] fix potential jiffies overflow

[Andrew Morton]

Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
>
> >>>>> On Thu, 2 Mar 2006 10:43:12 -0600, "Ram Gupta" <ram.gupta5@gmail.com> said:
> >> I found i386 timer_resume is updating jiffies, not jiffies_64.  It
> >> looks there is a potential overflow problem.  Is this a correct
> >> fix?
> 
> ram> The 64-bit jiffies value is not atomic. You need to hold
> ram> xtime_lock to read it.
> 
> OK, and I guess wall_jiffies also needs xtime_lock.
> 
> 
> I found i386 timer_resume is updating jiffies, not jiffies_64.  It
> looks there is a potential overflow problem.  And jiffies_64 and
> wall_jiffies should be protected by xtime_lock.
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> 
> diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
> index a14d594..9d30747 100644
> --- a/arch/i386/kernel/time.c
> +++ b/arch/i386/kernel/time.c
> @@ -412,9 +412,9 @@ static int timer_resume(struct sys_devic
>  	write_seqlock_irqsave(&xtime_lock, flags);
>  	xtime.tv_sec = sec;
>  	xtime.tv_nsec = 0;
> -	write_sequnlock_irqrestore(&xtime_lock, flags);
> -	jiffies += sleep_length;
> +	jiffies_64 += sleep_length;
>  	wall_jiffies += sleep_length;
> +	write_sequnlock_irqrestore(&xtime_lock, flags);
>  	if (last_timer->resume)
>  		last_timer->resume();
>  	cur_timer = last_timer;

Thanks, that looks like 2.6.16 material.

What happens if the machine slept for more than 49.7 days?

Re: [PATCH] fix potential jiffies overflow

[Atsushi Nemoto]

>>>>> On Thu, 2 Mar 2006 18:45:02 -0800, Andrew Morton <akpm@osdl.org> said:
akpm> Thanks, that looks like 2.6.16 material.

akpm> What happens if the machine slept for more than 49.7 days?

Well, jiffies will lose 49.7 days...  Then, how about this?  We can
sleep 136 years.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
index a14d594..be5d079 100644
--- a/arch/i386/kernel/time.c
+++ b/arch/i386/kernel/time.c
@@ -400,7 +400,7 @@ static int timer_resume(struct sys_devic
 {
 	unsigned long flags;
 	unsigned long sec;
-	unsigned long sleep_length;
+	u64 sleep_length;
 
 #ifdef CONFIG_HPET_TIMER
 	if (is_hpet_enabled())
@@ -408,7 +408,7 @@ static int timer_resume(struct sys_devic
 #endif
 	setup_pit_timer();
 	sec = get_cmos_time() + clock_cmos_diff;
-	sleep_length = (get_cmos_time() - sleep_start) * HZ;
+	sleep_length = (u64)(get_cmos_time() - sleep_start) * HZ;
 	write_seqlock_irqsave(&xtime_lock, flags);
 	xtime.tv_sec = sec;
 	xtime.tv_nsec = 0;

Re: [PATCH] fix potential jiffies overflow

[Andrew Morton]

Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
>
> >>>>> On Thu, 2 Mar 2006 18:45:02 -0800, Andrew Morton <akpm@osdl.org> said:
> akpm> Thanks, that looks like 2.6.16 material.
> 
> akpm> What happens if the machine slept for more than 49.7 days?
> 
> Well, jiffies will lose 49.7 days...  Then, how about this?  We can
> sleep 136 years.
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> 
> diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
> index a14d594..be5d079 100644
> --- a/arch/i386/kernel/time.c
> +++ b/arch/i386/kernel/time.c
> @@ -400,7 +400,7 @@ static int timer_resume(struct sys_devic
>  {
>  	unsigned long flags;
>  	unsigned long sec;
> -	unsigned long sleep_length;
> +	u64 sleep_length;
>  
>  #ifdef CONFIG_HPET_TIMER
>  	if (is_hpet_enabled())
> @@ -408,7 +408,7 @@ static int timer_resume(struct sys_devic
>  #endif
>  	setup_pit_timer();
>  	sec = get_cmos_time() + clock_cmos_diff;
> -	sleep_length = (get_cmos_time() - sleep_start) * HZ;
> +	sleep_length = (u64)(get_cmos_time() - sleep_start) * HZ;
>  	write_seqlock_irqsave(&xtime_lock, flags);
>  	xtime.tv_sec = sec;
>  	xtime.tv_nsec = 0;

but...

	wall_jiffies += sleep_length;

wall_jiffies is 32-bit.

Re: [PATCH] fix potential jiffies overflow

[Atsushi Nemoto]

>>>>> On Thu, 2 Mar 2006 19:48:41 -0800, Andrew Morton <akpm@osdl.org> said:
>> Well, jiffies will lose 49.7 days...  Then, how about this?  We can
>> sleep 136 years.

akpm> but...

akpm> 	wall_jiffies += sleep_length;

akpm> wall_jiffies is 32-bit.

Yes ...  and I think wall_jiffies can be removed completely (with
update_times() cleanup).  Of course it should not be 2.6.16 material.

---
Atsushi Nemoto

Re: [PATCH] fix potential jiffies overflow

[Matt Mackall]

On Fri, Mar 03, 2006 at 12:31:10PM +0900, Atsushi Nemoto wrote:
> >>>>> On Thu, 2 Mar 2006 18:45:02 -0800, Andrew Morton <akpm@osdl.org> said:
>  	sec = get_cmos_time() + clock_cmos_diff;
> -	sleep_length = (get_cmos_time() - sleep_start) * HZ;
> +	sleep_length = (u64)(get_cmos_time() - sleep_start) * HZ;

Calls to get_cmos_time() currently take an average of .5 seconds as it
waits for a clock edge to go by. I've got some patches queueing up to
eliminate this but it's probably worth caching the result here anyway.

-- 
Mathematics is the supreme nostalgia of our time.