Re: itimer oddness in 2.6.12
diff mbox series

Message ID 42E1A208.8060408@mvista.com
State New, archived
Headers show
Series
  • Re: itimer oddness in 2.6.12
Related show

Commit Message

George Anzinger July 23, 2005, 1:48 a.m. UTC
Tom Marshall wrote:
> On Fri, Jul 22, 2005 at 08:21:25PM +0100, Paulo Marques wrote:
> 
>>Tom Marshall wrote:
>>
>>>The patch to fix "setitimer timer expires too early" is causing issues for
>>>the Helix server.  We have a timer processs that updates the server's
>>>timestamp on an itimer and it expects the signal to be delivered at roughly
>>>the interval retrieved from getitimer.  This is very consistent on every
>>>platform, including Linux up to 2.6.11, but breaks on 2.6.12.  On 2.6.12,
>>>setting the itimer to 10ms and retrieving the actual interval from 
>>>getitimer
>>>reports 10.998ms, but the timer interrupts are consistently delivered at
>>>roughly 11.998ms.  
>>
>>Unfortunately, this is not so clear cut as it seems :(

Oops!  That patch is wrong.  The +1 should be applied to the initial 
interval _only_.  We KNOW when the repeating intervals start (i.e. at 
the jiffie edge) and don't need to adjust them.  The patch, however, 
incorrectly, rolls them all into one.  The attach patch should fix the 
problem.  Warnning, it compiles and boots, but I have not tested it.

George

Comments

Paulo Marques July 25, 2005, 11:58 a.m. UTC | #1
George Anzinger wrote:
> Tom Marshall wrote:
>> On Fri, Jul 22, 2005 at 08:21:25PM +0100, Paulo Marques wrote:
>>>[...]
>>> Unfortunately, this is not so clear cut as it seems :(
> 
> Oops!  That patch is wrong.  The +1 should be applied to the initial 
> interval _only_.  We KNOW when the repeating intervals start (i.e. at 
> the jiffie edge) and don't need to adjust them.  The patch, however, 
> incorrectly, rolls them all into one.  The attach patch should fix the 
> problem.  Warnning, it compiles and boots, but I have not tested it.

Yes, this seems to be the Right Thing :)

Acked-by: Paulo Marques <pmarques@grupopie.com>
Bill Davidsen July 25, 2005, 7:04 p.m. UTC | #2
George Anzinger wrote:
> Tom Marshall wrote:
> 
>> On Fri, Jul 22, 2005 at 08:21:25PM +0100, Paulo Marques wrote:
>>
>>> Tom Marshall wrote:
>>>
>>>> The patch to fix "setitimer timer expires too early" is causing 
>>>> issues for
>>>> the Helix server.  We have a timer processs that updates the server's
>>>> timestamp on an itimer and it expects the signal to be delivered at 
>>>> roughly
>>>> the interval retrieved from getitimer.  This is very consistent on 
>>>> every
>>>> platform, including Linux up to 2.6.11, but breaks on 2.6.12.  On 
>>>> 2.6.12,
>>>> setting the itimer to 10ms and retrieving the actual interval from 
>>>> getitimer
>>>> reports 10.998ms, but the timer interrupts are consistently 
>>>> delivered at
>>>> roughly 11.998ms.  
>>>
>>>
>>> Unfortunately, this is not so clear cut as it seems :(
> 
> 
> Oops!  That patch is wrong.  The +1 should be applied to the initial 
> interval _only_.  We KNOW when the repeating intervals start (i.e. at 
> the jiffie edge) and don't need to adjust them.  The patch, however, 
> incorrectly, rolls them all into one.  The attach patch should fix the 
> problem.  Warnning, it compiles and boots, but I have not tested it.

Can this get into 2.6.13? Or stable if it's too late? This would appear 
to be a fix to a visible problem.
Andrew Morton July 26, 2005, 6:17 a.m. UTC | #3
George Anzinger <george@mvista.com> wrote:
>
> +	while (time_before_eq(p->signal->real_timer.expires, jiffies))
> +		p->signal->real_timer.expires += inc;

It gives me the creeps when I see timer code doing this, and it seems to be
done relatively frequently.

Surely it can be calculated arithmetically?  If not, are you really sure
that it is not exploitable by malicious code?
-
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 July 26, 2005, 2:36 p.m. UTC | #4
Andrew Morton wrote:
> George Anzinger <george@mvista.com> wrote:
> 
>>+	while (time_before_eq(p->signal->real_timer.expires, jiffies))
>>+		p->signal->real_timer.expires += inc;
> 
> 
> It gives me the creeps when I see timer code doing this, and it seems to be
> done relatively frequently.
> 
> Surely it can be calculated arithmetically?  If not, are you really sure
> that it is not exploitable by malicious code?

Hm.. the system only falls into a loop here if the system is loaded to 
the point where we are a jiffie or more late.  The prior code just did 
the "+=" and called add_timer, possibly with a time in the past.  I 
suspect that way of doing this would never catch up if the user asked 
for a one jiffie repeat time.  Also, this is faster than the div, mpy if 
you are not late (or even if you are several jiffies late).

A possible alternative might be:
	p->signal->real_timer.expires += inc;	
	if (time_before_eq(p->signal->real_timer.expires, jiffies))
		p->signal->real_timer.expires += ((jiffies - 
p->signal->real_timer.expires + inc -1) / inc) * inc;

Both a div and a mpy in there.  I really think the "while" is ok, but if 
you prefer...	

The last time you questioned this sort of thing was in the code to 
correct an absolute timer.  In that case we were adjusting after a clock 
set and, yes, it was possibly exploitable (assuming you could set the 
clock).  Here we don't have that possibility, i.e. we only get into the 
loop if the system is late.
> -

Patch
diff mbox series

Index: linux-2.6.13-rc/kernel/itimer.c
===================================================================
--- linux-2.6.13-rc.orig/kernel/itimer.c
+++ linux-2.6.13-rc/kernel/itimer.c
@@ -112,28 +112,11 @@  asmlinkage long sys_getitimer(int which,
 	return error;
 }
 
-/*
- * Called with P->sighand->siglock held and P->signal->real_timer inactive.
- * If interval is nonzero, arm the timer for interval ticks from now.
- */
-static inline void it_real_arm(struct task_struct *p, unsigned long interval)
-{
-	p->signal->it_real_value = interval; /* XXX unnecessary field?? */
-	if (interval == 0)
-		return;
-	if (interval > (unsigned long) LONG_MAX)
-		interval = LONG_MAX;
-	/* the "+ 1" below makes sure that the timer doesn't go off before
-	 * the interval requested. This could happen if
-	 * time requested % (usecs per jiffy) is more than the usecs left
-	 * in the current jiffy */
-	p->signal->real_timer.expires = jiffies + interval + 1;
-	add_timer(&p->signal->real_timer);
-}
 
 void it_real_fn(unsigned long __data)
 {
 	struct task_struct * p = (struct task_struct *) __data;
+	unsigned long inc = p->signal->it_real_incr;
 
 	send_group_sig_info(SIGALRM, SEND_SIG_PRIV, p);
 
@@ -141,14 +124,23 @@  void it_real_fn(unsigned long __data)
 	 * Now restart the timer if necessary.  We don't need any locking
 	 * here because do_setitimer makes sure we have finished running
 	 * before it touches anything.
+	 * Note, we KNOW we are (or should be) at a jiffie edge here so 
+	 * we don't need the +1 stuff.  Also, we want to use the prior
+	 * expire value so as to not "slip" a jiffie if we are late.
+	 * Deal with requesting a time prior to "now" here rather than
+	 * in add_timer.
 	 */
-	it_real_arm(p, p->signal->it_real_incr);
+	if (!inc)
+		return;
+	while (time_before_eq(p->signal->real_timer.expires, jiffies))
+		p->signal->real_timer.expires += inc;
+	add_timer(&p->signal->real_timer);	
 }
 
 int do_setitimer(int which, struct itimerval *value, struct itimerval *ovalue)
 {
 	struct task_struct *tsk = current;
- 	unsigned long val, interval;
+ 	unsigned long val, interval, expires;
 	cputime_t cval, cinterval, nval, ninterval;
 
 	switch (which) {
@@ -160,7 +152,10 @@  int do_setitimer(int which, struct itime
 			del_timer_sync(&tsk->signal->real_timer);
 		tsk->signal->it_real_incr =
 			timeval_to_jiffies(&value->it_interval);
-		it_real_arm(tsk, timeval_to_jiffies(&value->it_value));
+		expires = timeval_to_jiffies(&value->it_value);
+		if (expires)
+			mod_timer(&tsk->signal->real_timer,
+				  jiffies + 1 + expires);
 		spin_unlock_irq(&tsk->sighand->siglock);
 		if (ovalue) {
 			jiffies_to_timeval(val, &ovalue->it_value);