del_timer_sync needed for UP RT systems.

del_timer_sync needed for UP RT systems.

[George Anzinger]

Ingo,

In tracking down the failure of a system running the RT patch we have found a 
preemption between the time run_timer_list clears its spinlock and the call back 
function (in this case in posix-timers.c) gets its spinlock.  The bad news is 
that it is possible for the timer to be released at this point leaving the call 
back code with a pointer to a bogus timer.

This was/is possible, of course, in SMP systems and is why del_timer_sync() 
exists.  I suspect that del_timer_sync() needs to also do the "right thing" in 
UP RT systems.

This means removing the #ifdef CONFIG_SMP at about line 56 of kernel/timer.c 
thus setting up base->running_timer in all cases (or at least in SMP and RT 
cases) and also the #ifdef CONFIG_SMP around del_timer_sync() and, of course, 
the defines that redirect calls to these functions.

Does this make sense?
-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

Re: del_timer_sync needed for UP RT systems.

[Daniel Walker]

[-- Attachment #1: Type: text/plain, Size: 1974 bytes --]

Basically , there is a race condition in sys_timer_delete() and
posix_timer_event() .

>From sys_timer_delete() :

   /*
	 * This keeps any tasks waiting on the spin lock from thinking
	 * they got something (see the lock code above).
	 */
	if (timer->it_process) {
		if (timer->it_sigev_notify ==
(SIGEV_SIGNAL|SIGEV_THREAD_ID))
			put_task_struct(timer->it_process);
		timer->it_process = NULL;
	}
	unlock_timer(timer, flags);
	/* Preemption happens here. */
	release_posix_timer(timer, IT_ID_SET);


So when the timer is getting triggered , right before it takes the timer
lock, preemption happens. You finish the code above. Then your preempted
again right after unlock timer, shown above.

At this point, your triggering a timer that is half deleted, in posix_timer_fn() .
timer->it_process = NULL , so when you try to send the signal to the
timer owner you crash with an OOPS , cause the timer owner was just set
to NULL.

George, at least CC me, after all I found/documented this bug ..

Preliminary fix included ..

Daniel

On Tue, 2005-04-26 at 16:42, George Anzinger wrote:
> Ingo,
> 
> In tracking down the failure of a system running the RT patch we have found a 
> preemption between the time run_timer_list clears its spinlock and the call back 
> function (in this case in posix-timers.c) gets its spinlock.  The bad news is 
> that it is possible for the timer to be released at this point leaving the call 
> back code with a pointer to a bogus timer.
> 
> This was/is possible, of course, in SMP systems and is why del_timer_sync() 
> exists.  I suspect that del_timer_sync() needs to also do the "right thing" in 
> UP RT systems.
> 
> This means removing the #ifdef CONFIG_SMP at about line 56 of kernel/timer.c 
> thus setting up base->running_timer in all cases (or at least in SMP and RT 
> cases) and also the #ifdef CONFIG_SMP around del_timer_sync() and, of course, 
> the defines that redirect calls to these functions.
> 
> Does this make sense?

[-- Attachment #2: fix_posix_timer_half_delete.patch --]
[-- Type: text/plain, Size: 1550 bytes --]

Source: MontaVista Software, Inc.
MR: 11506 
Type: Defect Fix
Disposition: needs submitting to LKML 
Signed-off-by: Daniel Walker <dwalker@mvista.com>
Description:
	Ok, so here is the run down.. Basically , there is a race condition in
sys_timer_delete() and posix_timer_event() .

>From sys_timer_delete() :
		timer->it_process = NULL;
	}
	unlock_timer(timer, flags);
	/* Preemption happens here. */
	release_posix_timer(timer, IT_ID_SET);


So when the timer is getting triggered , right before it takes the timer
lock, preemption happens. You finish the code above. Then your preempted
again right after unlock timer, shown above.

At this point, your triggering a timer that is half deleted, in posix_timer_fn() .
timer->it_process = NULL , so when you try to send the signal to the
timer owner you crash with an OOPS , because the timer owner was just set
to NULL.

Index: linux-2.6.10/kernel/posix-timers.c
===================================================================
--- linux-2.6.10.orig/kernel/posix-timers.c	2005-04-26 17:38:25.000000000 +0000
+++ linux-2.6.10/kernel/posix-timers.c	2005-04-26 17:53:54.000000000 +0000
@@ -433,6 +433,14 @@ exit:
 
 int posix_timer_event(struct k_itimer *timr,int si_private)
 {
+	/*
+	 * If it_process is NULL then this timer is
+	 * in the process of being deleted. At this
+	 * point we can't do very much. So we
+	 * try to return gracefuly.
+	 */
+	if (timr->it_process == NULL) return 1;
+
 	memset(&timr->sigq->info, 0, sizeof(siginfo_t));
 	timr->sigq->info.si_sys_private = si_private;
 	/*

Re: del_timer_sync needed for UP RT systems.

[George Anzinger]

Daniel Walker wrote:
> Basically , there is a race condition in sys_timer_delete() and
> posix_timer_event() .
> 
>>From sys_timer_delete() :
> 
>    /*
> 	 * This keeps any tasks waiting on the spin lock from thinking
> 	 * they got something (see the lock code above).
> 	 */
> 	if (timer->it_process) {
> 		if (timer->it_sigev_notify ==
> (SIGEV_SIGNAL|SIGEV_THREAD_ID))
> 			put_task_struct(timer->it_process);
> 		timer->it_process = NULL;
> 	}
> 	unlock_timer(timer, flags);
> 	/* Preemption happens here. */
> 	release_posix_timer(timer, IT_ID_SET);
> 
> 
> So when the timer is getting triggered , right before it takes the timer
> lock, preemption happens. You finish the code above. Then your preempted
> again right after unlock timer, shown above.
> 
> At this point, your triggering a timer that is half deleted, in posix_timer_fn() .
> timer->it_process = NULL , so when you try to send the signal to the
> timer owner you crash with an OOPS , cause the timer owner was just set
> to NULL.
> 
> George, at least CC me, after all I found/documented this bug ..

Sorry, my bad :(
> 
> Preliminary fix included ..
> 
> Daniel
> 
> On Tue, 2005-04-26 at 16:42, George Anzinger wrote:
> 
>>Ingo,
>>
>>In tracking down the failure of a system running the RT patch we have found a 
>>preemption between the time run_timer_list clears its spinlock and the call back 
>>function (in this case in posix-timers.c) gets its spinlock.  The bad news is 
>>that it is possible for the timer to be released at this point leaving the call 
>>back code with a pointer to a bogus timer.
>>
>>This was/is possible, of course, in SMP systems and is why del_timer_sync() 
>>exists.  I suspect that del_timer_sync() needs to also do the "right thing" in 
>>UP RT systems.
>>
>>This means removing the #ifdef CONFIG_SMP at about line 56 of kernel/timer.c 
>>thus setting up base->running_timer in all cases (or at least in SMP and RT 
>>cases) and also the #ifdef CONFIG_SMP around del_timer_sync() and, of course, 
>>the defines that redirect calls to these functions.
>>
>>Does this make sense?
>>
>>
>>------------------------------------------------------------------------
>>
>>Source: MontaVista Software, Inc.
>>MR: 11506 
>>Type: Defect Fix
>>Disposition: needs submitting to LKML 
>>Signed-off-by: Daniel Walker <dwalker@mvista.com>
>>Description:
>>	Ok, so here is the run down.. Basically , there is a race condition in
>>sys_timer_delete() and posix_timer_event() .
>>
>>>From sys_timer_delete() :
>>		timer->it_process = NULL;
>>	}
>>	unlock_timer(timer, flags);
>>	/* Preemption happens here. */
>>	release_posix_timer(timer, IT_ID_SET);
>>
>>
>>So when the timer is getting triggered , right before it takes the timer
>>lock, preemption happens. You finish the code above. Then your preempted
>>again right after unlock timer, shown above.
>>
>>At this point, your triggering a timer that is half deleted, in posix_timer_fn() .
>>timer->it_process = NULL , so when you try to send the signal to the
>>timer owner you crash with an OOPS , because the timer owner was just set
>>to NULL.
>>
>>Index: linux-2.6.10/kernel/posix-timers.c
>>===================================================================
>>--- linux-2.6.10.orig/kernel/posix-timers.c	2005-04-26 17:38:25.000000000 +0000
>>+++ linux-2.6.10/kernel/posix-timers.c	2005-04-26 17:53:54.000000000 +0000
>>@@ -433,6 +433,14 @@ exit:
>> 
>> int posix_timer_event(struct k_itimer *timr,int si_private)
>> {
>>+	/*
>>+	 * If it_process is NULL then this timer is
>>+	 * in the process of being deleted. At this
>>+	 * point we can't do very much. So we
>>+	 * try to return gracefuly.
>>+	 */
>>+	if (timr->it_process == NULL) return 1;
>>+
>> 	memset(&timr->sigq->info, 0, sizeof(siginfo_t));
>> 	timr->sigq->info.si_sys_private = si_private;
>> 	/*
The problem here is that the reference is to timr, a pointer to something which 
has been deleted.  The memory may well be used elsewhere by this time which will 
make the test of it_process wrong.  It also means we could mess with someone 
elses memory in the memset above.
-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

Re: del_timer_sync needed for UP RT systems.

[Daniel Walker]

On Tue, 2005-04-26 at 17:14, George Anzinger wrote:

> The problem here is that the reference is to timr, a pointer to something which 
> has been deleted.  The memory may well be used elsewhere by this time which will 
> make the test of it_process wrong.  It also means we could mess with someone 
> elses memory in the memset above.

Bottom line, you can use sys_timer_delete() on a timer, and trigger the
same timer your deleting .. Those operations should be serialized, which
they currently aren't .. 


Daniel

Re: del_timer_sync needed for UP RT systems.

[Daniel Walker]

On Tue, 2005-04-26 at 17:24, Daniel Walker wrote:
> On Tue, 2005-04-26 at 17:14, George Anzinger wrote:
> 
> > The problem here is that the reference is to timr, a pointer to something which 
> > has been deleted.  The memory may well be used elsewhere by this time which will 
> > make the test of it_process wrong.  It also means we could mess with someone 
> > elses memory in the memset above.
> 
> Bottom line, you can use sys_timer_delete() on a timer, and trigger the
> same timer your deleting .. Those operations should be serialized, which
> they currently aren't .. 

Um, sorry typo, you _can't_ delete a timer, and trigger it at the same
time. That's bad.

Daniel

Re: del_timer_sync needed for UP RT systems.

[George Anzinger]

Daniel Walker wrote:
> On Tue, 2005-04-26 at 17:14, George Anzinger wrote:
> 
> 
>>The problem here is that the reference is to timr, a pointer to something which 
>>has been deleted.  The memory may well be used elsewhere by this time which will 
>>make the test of it_process wrong.  It also means we could mess with someone 
>>elses memory in the memset above.
> 
> 
> Bottom line, you can use sys_timer_delete() on a timer, and trigger the
> same timer your deleting .. Those operations should be serialized, which
> they currently aren't .. 

I agree.  The change to do this is to use the del_timer_sync() or the 
del_singleshot_timer() code.

It is possible and desirable to be able to delete a running timer.  We don't 
want to take it away from the timer call back routine, however, as that leads to 
"bad things".  That is why these two del_* routines were written.
> 
-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

Re: del_timer_sync needed for UP RT systems.

[Daniel Walker]

On Tue, 2005-04-26 at 17:34, George Anzinger wrote:
> I agree.  The change to do this is to use the del_timer_sync() or the 
> del_singleshot_timer() code.
> 
> It is possible and desirable to be able to delete a running timer.  We don't 
> want to take it away from the timer call back routine, however, as that leads to 
> "bad things".  That is why these two del_* routines were written.


I'll defer to you on that, since I don't really know what timer people
need.. 

After reviewing, del_timer_sync() I don't think that will stop this
race. Because the wakeup happens before posix_timer_fn() is called in
__run_timers() .

Daniel