[RTEMS Project] #2310: Race condition in _Thread_Change_priority()

[RTEMS trac]

#2310: Race condition in _Thread_Change_priority()
-----------------------------+---------------------
 Reporter:  sebastian.huber  |       Owner:
     Type:  defect           |      Status:  new
 Priority:  normal           |   Milestone:  4.11.1
Component:  cpukit           |     Version:  4.11
 Severity:  critical         |  Resolution:
 Keywords:                   |
-----------------------------+---------------------
Description changed by sebastian.huber:

Old description:

> We have:
>
> void _Thread_Change_priority(
>   Thread_Control   *the_thread,
>   Priority_Control  new_priority,
>   bool              prepend_it
> )
> {
>   /*
>    *  Do not bother recomputing all the priority related information if
>    *  we are not REALLY changing priority.
>    */
>   if ( the_thread->current_priority != new_priority ) {
>     ISR_Level level;
>
>     _ISR_Disable( level );
>
>     the_thread->current_priority = new_priority;
>
>     if ( _States_Is_ready( the_thread->current_state ) ) {
>       _Scheduler_Change_priority(
>         the_thread,
>         new_priority,
>         prepend_it
>       );
>     } else {
>       _Scheduler_Update_priority( the_thread, new_priority );
>     }
>
>     _ISR_Enable( level );
>
> <-- Here we changed the the_thread->current_priority and provided the
> thread is in a priority queue (RB tree), then its position in the tree is
> wrong now. In case it is the first or last element in the tree, then
> extract operations may corrupt the tree (e.g. if a timeout happens now).
>
>     _Thread_queue_Requeue( the_thread->Wait.queue, the_thread );
>   }
> }
>
> A potential solution is to set the_thread->current_priority and call
> _Thread_queue_Requeue() in one critical section.  The scheduler is more
> resilient to temporarily wrong priority values, since it maintains its
> own priority depending state.

New description:

 We have:

 {{{
 #!c
 void _Thread_Change_priority(
   Thread_Control   *the_thread,
   Priority_Control  new_priority,
   bool              prepend_it
 )
 {
   /*
    *  Do not bother recomputing all the priority related information if
    *  we are not REALLY changing priority.
    */
   if ( the_thread->current_priority != new_priority ) {
     ISR_Level level;

     _ISR_Disable( level );

     the_thread->current_priority = new_priority;

     if ( _States_Is_ready( the_thread->current_state ) ) {
       _Scheduler_Change_priority(
         the_thread,
         new_priority,
         prepend_it
       );
     } else {
       _Scheduler_Update_priority( the_thread, new_priority );
     }

     _ISR_Enable( level );
 }}}

 Here we changed the the_thread->current_priority and provided the thread
 is in a priority queue (RB tree), then its position in the tree is wrong
 now. In case it is the first or last element in the tree, then extract
 operations may corrupt the tree (e.g. if a timeout happens now).

 {{{
 #!c
     _Thread_queue_Requeue( the_thread->Wait.queue, the_thread );
   }
 }
 }}}

 A potential solution is to set the_thread->current_priority and call
 _Thread_queue_Requeue() in one critical section.  The scheduler is more
 resilient to temporarily wrong priority values, since it maintains its own
 priority depending state.

--

--
Ticket URL: <http://devel.rtems.org/ticket/2310#comment:1>
RTEMS Project <http://www.rtems.org/>
RTEMS Project