[ofw] [PATCH v2] complib/timer: fix race accessing timer fields

[Hefty, Sean]

The timeout_time field of cl_timer_t is modified in the timer
callback function and also in calls to cl_timer_start, cl_timer_stop,
and cl_timer_trim.  The user cannot protect against the changes
made in the timer callback function, so the timer must provide
this protection itself.

Provide serialization to prevent multiple timer callbacks to
the user at once.

Signed-off-by: Sean Hefty <sean.hefty at intel.com>
---
changes from v1: used Windows spinlock calls rather than abstractions
compile tested only

 trunk/core/complib/kernel/cl_timer.c    |   41 +++++++++++++++++++++++--------
 trunk/inc/kernel/complib/cl_timer_osd.h |    2 ++
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/trunk/core/complib/kernel/cl_timer.c b/trunk/core/complib/kernel/cl_timer.c
index 34cae8e..e235642 100644
--- a/trunk/core/complib/kernel/cl_timer.c
+++ b/trunk/core/complib/kernel/cl_timer.c
@@ -41,13 +41,19 @@ __timer_callback(
 	IN	void*		arg1,
 	IN	void*		arg2 )
 {
+	KLOCK_QUEUE_HANDLE hlock;
+
 	UNUSED_PARAM( p_dpc );
 	UNUSED_PARAM( arg1 );
 	UNUSED_PARAM( arg2 );
 
+	KeAcquireInStackQueuedSpinLockAtDpcLevel( &p_timer->spinlock, &hlock );
 	p_timer->timeout_time = 0;
+	KeReleaseInStackQueuedSpinLockFromDpcLevel( &hlock );
 
+	KeAcquireInStackQueuedSpinLockAtDpcLevel( &p_timer->cb_lock, &hlock );
 	(p_timer->pfn_callback)( (void*)p_timer->context );
+	KeReleaseInStackQueuedSpinLockFromDpcLevel( &hlock );
 }
 
 
@@ -75,6 +81,8 @@ cl_timer_init(
 
 	KeInitializeTimer( &p_timer->timer );
 	KeInitializeDpc( &p_timer->dpc, __timer_callback, p_timer );
+	KeInitializeSpinLock( &p_timer->spinlock );
+	KeInitializeSpinLock( &p_timer->cb_lock );
 
 	return( CL_SUCCESS );
 }
@@ -108,6 +116,8 @@ cl_timer_start(
 	IN	const uint32_t		time_ms )
 {
 	LARGE_INTEGER	due_time;
+	uint64_t		timeout_time;
+	KLOCK_QUEUE_HANDLE hlock;
 
 	CL_ASSERT( p_timer );
 	CL_ASSERT( p_timer->pfn_callback );
@@ -115,11 +125,13 @@ cl_timer_start(
 
 	/* Due time is in 100 ns increments.  Negative for relative time. */
 	due_time.QuadPart = -(int64_t)(((uint64_t)time_ms) * 10000);
+	timeout_time = cl_get_time_stamp() + (((uint64_t)time_ms) * 1000);
 
-	/* Store the timeout time in the timer object. */
-	p_timer->timeout_time = cl_get_time_stamp() + (((uint64_t)time_ms) * 1000);
-
+	KeAcquireInStackQueuedSpinLock( &p_timer->spinlock, &hlock );
+	p_timer->timeout_time = timeout_time;
 	KeSetTimer( &p_timer->timer, due_time, &p_timer->dpc );
+	KeReleaseInStackQueuedSpinLock( &hlock );
+
 	return( CL_SUCCESS );
 }
 
@@ -129,19 +141,25 @@ cl_timer_trim(
 	IN	cl_timer_t* const	p_timer,
 	IN	const uint32_t		time_ms )
 {
+	LARGE_INTEGER	due_time;
 	uint64_t		timeout_time;
+	KLOCK_QUEUE_HANDLE hlock;
 
 	CL_ASSERT( p_timer );
 	CL_ASSERT( p_timer->pfn_callback );
 
-	/* Calculate the timeout time in the timer object. */
 	timeout_time = cl_get_time_stamp() + (((uint64_t)time_ms) * 1000);
 
 	/* Only pull in the timeout time. */
-	if( p_timer->timeout_time && p_timer->timeout_time < timeout_time )
-		return( CL_SUCCESS );
-
-	return cl_timer_start( p_timer, time_ms );
+	KeAcquireInStackQueuedSpinLock( &p_timer->spinlock, &hlock );
+	if( !p_timer->timeout_time || p_timer->timeout_time > timeout_time )
+	{
+		p_timer->timeout_time = timeout_time;
+		due_time.QuadPart = -(int64_t)(((uint64_t)time_ms) * 10000);
+		KeSetTimer( &p_timer->timer, due_time, &p_timer->dpc );
+	}
+	KeReleaseInStackQueuedSpinLock( &hlock );
+	return( CL_SUCCESS );
 }
 
 
@@ -149,12 +167,15 @@ void
 cl_timer_stop(
 	IN	cl_timer_t* const	p_timer )
 {
+	KLOCK_QUEUE_HANDLE hlock;
+
 	CL_ASSERT( p_timer );
 	CL_ASSERT( p_timer->pfn_callback );
 	CL_ASSERT( KeGetCurrentIrql() <= DISPATCH_LEVEL );
 	
 	/* Cancel the timer.  This also cancels any queued DPCs for the timer. */
-	KeCancelTimer( &p_timer->timer );
-
+	KeAcquireInStackQueuedSpinLock( &p_timer->spinlock, &hlock );
 	p_timer->timeout_time = 0;
+	KeCancelTimer( &p_timer->timer );
+	KeReleaseInStackQueuedSpinLock( &hlock );
 }
diff --git a/trunk/inc/kernel/complib/cl_timer_osd.h b/trunk/inc/kernel/complib/cl_timer_osd.h
index 4cb99b0..0935902 100644
--- a/trunk/inc/kernel/complib/cl_timer_osd.h
+++ b/trunk/inc/kernel/complib/cl_timer_osd.h
@@ -48,6 +48,8 @@ typedef struct _cl_timer
 	cl_pfn_timer_callback_t	pfn_callback;
 	const void				*context;
 	uint64_t				timeout_time;
+	KSPIN_LOCK				spinlock;
+	KSPIN_LOCK				cb_lock;
 
 } cl_timer_t;
 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: cl_timer
Type: application/octet-stream
Size: 4501 bytes
Desc: cl_timer
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20100623/b5b7e610/attachment.obj>