[ofw] [PATCH] 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.

Signed-off-by: Sean Hefty <sean.hefty at intel.com>
---
I looked at the kernel code based on what Tzachi found for the user
space implementation.  I can't say for certain that the lack of
synchronization causes an issue, but the timeout_time value
can definitely be modified outside of the user's control.  The
potential issue is that cl_timer_trim reads timeout_time
twice and writes it once; the timeout_time value can change
between the two reads, which leaves open the possibility that
cl_timer_trim will not set the timer.  The existing code may not
ever hit this, but it seems safer to provide the necessary
synchronization.

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

diff --git a/trunk/core/complib/kernel/cl_timer.c b/trunk/core/complib/kernel/cl_timer.c
index 34cae8e..ccbc794 100644
--- a/trunk/core/complib/kernel/cl_timer.c
+++ b/trunk/core/complib/kernel/cl_timer.c
@@ -45,7 +45,9 @@ __timer_callback(
 	UNUSED_PARAM( arg1 );
 	UNUSED_PARAM( arg2 );
 
+	cl_spinlock_acquire( &p_timer->spinlock );
 	p_timer->timeout_time = 0;
+	cl_spinlock_release( &p_timer->spinlock );
 
 	(p_timer->pfn_callback)( (void*)p_timer->context );
 }
@@ -75,6 +77,7 @@ cl_timer_init(
 
 	KeInitializeTimer( &p_timer->timer );
 	KeInitializeDpc( &p_timer->dpc, __timer_callback, p_timer );
+	cl_spinlock_init( &p_timer->spinlock );
 
 	return( CL_SUCCESS );
 }
@@ -108,6 +111,7 @@ cl_timer_start(
 	IN	const uint32_t		time_ms )
 {
 	LARGE_INTEGER	due_time;
+	uint64_t		timeout_time;
 
 	CL_ASSERT( p_timer );
 	CL_ASSERT( p_timer->pfn_callback );
@@ -115,11 +119,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);
-
+	cl_spinlock_acquire( &p_timer->spinlock );
+	p_timer->timeout_time = timeout_time;
 	KeSetTimer( &p_timer->timer, due_time, &p_timer->dpc );
+	cl_spinlock_release( &p_timer->spinlock );
+
 	return( CL_SUCCESS );
 }
 
@@ -129,19 +135,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;
 
 	CL_ASSERT( p_timer );
 	CL_ASSERT( p_timer->pfn_callback );
 
-	/* Calculate the timeout time in the timer object. */
+	/* 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);
 
 	/* 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 );
+	cl_spinlock_acquire( &p_timer->spinlock );
+	if( !p_timer->timeout_time || p_timer->timeout_time > timeout_time )
+	{
+		p_timer->timeout_time = timeout_time;
+		KeSetTimer( &p_timer->timer, due_time, &p_timer->dpc );
+	}
+	cl_spinlock_release( &p_timer->spinlock );
+	return( CL_SUCCESS );
 }
 
 
@@ -154,7 +166,8 @@ cl_timer_stop(
 	CL_ASSERT( KeGetCurrentIrql() <= DISPATCH_LEVEL );
 	
 	/* Cancel the timer.  This also cancels any queued DPCs for the timer. */
-	KeCancelTimer( &p_timer->timer );
-
+	cl_spinlock_acquire( &p_timer->spinlock );
 	p_timer->timeout_time = 0;
+	KeCancelTimer( &p_timer->timer );
+	cl_spinlock_release( &p_timer->spinlock );
 }
diff --git a/trunk/inc/kernel/complib/cl_timer_osd.h b/trunk/inc/kernel/complib/cl_timer_osd.h
index 4cb99b0..32fef83 100644
--- a/trunk/inc/kernel/complib/cl_timer_osd.h
+++ b/trunk/inc/kernel/complib/cl_timer_osd.h
@@ -38,7 +38,7 @@
 
 
 #include "complib/cl_types.h"
-
+#include "complib/cl_spinlock.h"
 
 /* Timer object definition. */
 typedef struct _cl_timer
@@ -48,6 +48,7 @@ typedef struct _cl_timer
 	cl_pfn_timer_callback_t	pfn_callback;
 	const void				*context;
 	uint64_t				timeout_time;
+	cl_spinlock_t			spinlock;
 
 } cl_timer_t;