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 <[email protected]> --- 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; _______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
