[BUG] Take the same mutex twice by the same task could cause an assertion failure.

[wirelinker]

Describe the bug
I occasionally found a way to trigger the assertion failure configASSERT( pxTCB != pxCurrentTCB ); at L6886
in vTaskPriorityDisinheritAfterTimeout().

And I tested the case on RP2040 (SMP) and ESP32-C3 (single core). Both of them will trigger the failure.

Target

• RP2040 on Pico_W
  • ARMv6-M
  • pico-SDK 2.2.0
  • FreeRTOS kernel: branch main @ 67f59a5
• ESP32-C3 on YD-ESP32-C3-D
  • RISC-V
  • ESP-IDF 5.5.2
  • FreeRTOS kernel: customized kernel modified by Espressif.

Host

• Ubuntu 24.04

To Reproduce

Test Case Description:

• Task1 priority = 1
• Task2 priority = 2
• mutex1 and mutex2

Test Steps:

• task1
  • take mutex1.
• task2
  • take mutex1 and block indefinitely.
• task1
  • take mutex2.
  • give mutex1.
• task2
  • unblock and take mutex1.
• task1
  • take mutex2 again and block itself for a period of time.
  • block timeout and vTaskPriorityDisinheritAfterTimeout be called.
• failed assertion triggered.

Explanation:

1. Task1 inherited the priority from task2 when holding mutex1.

2. Task1 take mutex2 then give mutex1, so the inherited priotiry
   won't disinherit back to the original priority. And the
   number of mutex held by task1 is only one.

3. Task1 take mutex2 again and block itself for a period of time.
   After timeout, the context switch calls xTaskIncrementTick to unblock
   task1. Then the "EventItem" of task1 will be removed from the list of
   waiting to take the mutex2. Now, the list is empty.

4. Then the vTaskPriorityDisinheritAfterTimeout will be called.

5. In vTaskPriorityDisinheritAfterTimeout function.

   • a. The uxHighestPriorityWaitingTask will be 0. Because the
     List of waiting to take the mutex2 is empty.
     • So the following will be set:
       • uxPriorityToUse = pxTCB->uxBasePriority;
   • b. The following will be true:
     • if( pxTCB->uxPriority != uxPriorityToUse )
       • Due to uxPriority != uxPriorityToUse == uxBasePriority
   • c. The following will be true:
     • if( pxTCB->uxMutexesHeld == uxOnlyOneMutexHeld )
       • Due to task1 only holds one mutex.
   • d. The following assertion will fail:
     • configASSERT( pxTCB != pxCurrentTCB );
       • Because task1 is the holder of mutex2 and the pxCurrentTCB == task1.

Test Result

• From RP2040:
  • test code: FreeRTOS_kernel_test_for_RP2040 @ inherit_then_take_same_mutex_again_test

task1 created mutex1.                                                                   
task1 took mutex1.                                                                      
task1 created task2.                                                                    
task2 on core 1, pri=2, base_pri=2, ct=1
task1 created mutex2.
task1 handle=0x20002848
task2 handle=0x200053E8
mutex1 handle=0x20004B80
mutex2 handle=0x20005470
task1 on core 0, pri=1, base_pri=1, ct=1
task2 on core 1, pri=2, base_pri=2, ct=2
task1 on core 1, pri=1, base_pri=1, ct=2
task2 on core 1, pri=2, base_pri=2, ct=3
task1 on core 1, pri=2, base_pri=1, ct=3
task1 on core 1, pri=2, base_pri=1, ct=4
task1 on core 1, pri=2, base_pri=1, ct=5
task1 took mutex2.
task1 gave mutex1.
task2 took mutex1.
task2 on core 1, pri=2, base_pri=2, ct=4
task1 on core 0, pri=2, base_pri=1, ct=6
handle=0x20002848, value=0
assertion "pxTCB != xTaskGetCurrentTaskHandle()" failed: file "FreeRTOS-Kernel/tasks.c", line 688assertion "( uxCi

• From ESP32-C3
  • test code: FreeRTOS_kernel_test_for_ESP32C3 @ inherit_then_take_same_mutex_again_test

Hello FreeRTOS test!
This is esp32c3 chip with 1 CPU core(s), WiFi/BLE, silicon revision v0.3, 4MB external flash
Minimum free heap size: 332256 bytes
===========================
Start FreeRTOS test
===========================
Create task_1.
task2 on core 0, pri=2, ct=1
Going to suspend startup main_task itself.
task1 took the mutex and created task2.
task1 created mutex2.
task1 on core 0, pri=1, ct=1
task2 on core 0, pri=2, ct=2
task1 on core 0, pri=2, ct=2
task1 on core 0, pri=2, ct=3
task1 on core 0, pri=2, ct=4
task1 on core 0, pri=2, ct=5
task1 took mutex2.
task1 gave mutex1.
task2 took mutex1.
task1 on core 0, pri=2, ct=6
task2 on core 0, pri=2, ct=3

assert failed: vTaskPriorityDisinheritAfterTimeout tasks.c:5270 (pxTCB != pxCurrentTCBs[ 0 ])
Core  0 register dump:
MEPC    : 0x40382f6c  RA      : 0x40382f2a  SP      : 0x3fc8fc60  GP      : 0x3fc8c000  
TP      : 0x3fc8fe10  T0      : 0x37363534  T1      : 0x7271706f  T2      : 0x33323130  
S0/FP   : 0x0000006a  S1      : 0x00000001  A0      : 0x3fc8fc9c  A1      : 0x3fc8c03d  
A2      : 0x00000001  A3      : 0x00000029  A4      : 0x00000001  A5      : 0x3fc8d000  
A6      : 0x7a797877  A7      : 0x76757473  S2      : 0x00000009  S3      : 0x3fc8fdcd  
S4      : 0x3fc8c03c  S5      : 0x00000000  S6      : 0x00000000  S7      : 0x00000000  
S8      : 0x00000000  S9      : 0x00000000  S10     : 0x00000000  S11     : 0x00000000  
T3      : 0x6e6d6c6b  T4      : 0x6a696867  T5      : 0x66656463  T6      : 0x62613938  
MSTATUS : 0x00001881  MTVEC   : 0x40380001  MCAUSE  : 0x00000002  MTVAL   : 0x00000000  
MHARTID : 0x00000000  

Expected behavior
The assertion should not be triggered.

Additional context
I considered two solutions:

• 1. Add one more check at if( pxMutexHolder != NULL ) @L6855
  • It would look like: if( ( pxMutexHolder != NULL ) && ( pxMutexHolder != pxCurrentTCB ))
  • This could avoid all the cases which mutex holder want to disinherit it's own priority.
  • But less efficiency. Because the mutex holder may keep an unnecessary high priority to prevent other tasks from execution.
  • This could keep the concept model of mutex that the mutex holder should not disinherit itself when the mutex still be held.
• 2. Just remove the configASSERT( pxTCB != pxCurrentTCB ); @L6886
  • I tested this solution. The priority of mutex holder was just downgraded to it's own base priority with no further problem.
  • Looks like the mutex holder is reasonable to do the following
    • check itself holds the only one mutex.
    • downgrade itself to the priority of the highest priority task waiting for the mutex or the mutex holder itself.

• I am not pretty sure which one is better. If we would like to be safe, the first solution could be considered.
  And I also found this: https://www.freertos.org/FreeRTOS_Support_Forum_Archive/December_2016/freertos_xTaskPriorityDisinherit_when_other_mutexes_are_held_4d6144d1j.html
  It said the implementation of mutex in FreeRTOS kernel is for simplicity. So the mutex doesn't keep detailed information about the previous priority change.

• I also tried to add some unit test in CMock folder:

• For solution 1.

  • By the way, solution 1 needs to correct other unit test cases. Because the current test cases are using only one task to do the test. So ( pxMutexHolder != pxCurrentTCB ) would prevent the test to do disinheritance. Create another dummy task could fix this.

void test_vTaskPriorityDisinheritAfterTimeout_success7()
{
    TaskHandle_t mutex_holder;
    UBaseType_t inheritedPriority = 2U;

    /* Setup */
    create_task_priority = 1;
    mutex_holder = create_task();
    mutex_holder->uxMutexesHeld = 1;
    mutex_holder->uxPriority = inheritedPriority;
    pxCurrentTCB = mutex_holder;
    /* Expectations */

    /* API Call */
    vTaskPriorityDisinheritAfterTimeout( mutex_holder,
                                         0 );
    /* Validations */
    TEST_ASSERT_EQUAL( inheritedPriority, mutex_holder->uxPriority );
}

• For solution 2.

void test_vTaskPriorityDisinheritAfterTimeout_success7()
{
    TaskHandle_t mutex_holder;
    UBaseType_t inheritedPriority = 2U;

    /* Setup */
    create_task_priority = 1;
    mutex_holder = create_task();
    mutex_holder->uxMutexesHeld = 1;
    mutex_holder->uxPriority = inheritedPriority;
    pxCurrentTCB = mutex_holder;
    /* Expectations */
    listGET_LIST_ITEM_VALUE_ExpectAndReturn( &mutex_holder->xEventListItem,  configMAX_PRIORITIES - inheritedPriority );
    listSET_LIST_ITEM_VALUE_Expect( &mutex_holder->xEventListItem, ( configMAX_PRIORITIES - mutex_holder->uxBasePriority ) );

    listIS_CONTAINED_WITHIN_ExpectAndReturn( &pxReadyTasksLists[ inheritedPriority ],
                                             &mutex_holder->xStateListItem,
                                             pdTRUE );
    uxListRemove_ExpectAndReturn( &mutex_holder->xStateListItem, 0 );
    /* prvAddTaskToReadyList */
    listINSERT_END_Expect( &pxReadyTasksLists[ mutex_holder->uxBasePriority ],
                           &mutex_holder->xStateListItem );
    /* API Call */
    vTaskPriorityDisinheritAfterTimeout( mutex_holder,
                                         0 );
    /* Validations */
    TEST_ASSERT_EQUAL( mutex_holder->uxBasePriority, mutex_holder->uxPriority );
}

That's what I've done so far. Thanks.

[aggarg]

Are you talking about taking the mutex again which a task already owns? If yes, you are supposed to use recursive mutex - https://www.freertos.org/Documentation/02-Kernel/04-API-references/10-Semaphore-and-Mutexes/08-xSemaphoreCreateRecursiveMutex.

[wirelinker]

The issue I described above is about "assertion failure", which would cause the system abnormally be trapped in some infinite loop or reset..

Try to take a non-recursive mutex by the same task repeatedly will "fail". Which means the mutex could not be taken again. But not "halt" the whole system. I had already tried this case with no problem.

The key point is the information(the inherited priority) be kept by a mutex holder, could be acquired when holding another mutex.

So the test case I described above is:

• 1. task1 inherited a higher priority when holding the mutex1.
• 2. task1 take mutex2, then give back mutex1.
• 3. task1 still keeps the inherited higher priority and holds only mutex2.
• Under this condition, if task1 try to take the mutex2 again and block until timeout. The system will halt, not just "fail" to take the mutex2.