From 1625a3aae210b605686fb202d3b66fa6a91486af Mon Sep 17 00:00:00 2001
From: Darian Leung <darian@espressif.com>
Date: Wed, 29 Jun 2022 00:58:08 +0800
Subject: [PATCH] freertos: Fix event group task list race condition

FreeRTOS synchronization primitives (e.g., queues, eventgroups) use various event lists (i.e., task lists) to track what
tasks are blocked on a current primitive. Usually these event lists are accessed via one of the event lists functions
(such as vTask[PlaceOn|RemoveFrom]UnorderedEventList()), which in turn ensure that the global task list spinlock
(xTaskQueueMutex) is taken when accessing these lists.

However, some functions in event_groups.c manually traverse their event lists. Thus if a tick interrupt occurs on
another core during traversal and that tick interrupt unblocks a task on the event list being traversed, the event list
will be corrupted.

This commit modifies the following event_groups.c functions so that they take the global task list lock before
traversing their event list.

- xEventGroupSetBits()
- vEventGroupDelete()
---
 .../freertos/FreeRTOS-Kernel/event_groups.c   | 16 ++++++++++++++++
 .../FreeRTOS-Kernel/include/freertos/task.h   | 19 +++++++++++++++++++
 components/freertos/FreeRTOS-Kernel/tasks.c   | 14 ++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/components/freertos/FreeRTOS-Kernel/event_groups.c b/components/freertos/FreeRTOS-Kernel/event_groups.c
index 1e2f7f7322..0d35f33244 100644
--- a/components/freertos/FreeRTOS-Kernel/event_groups.c
+++ b/components/freertos/FreeRTOS-Kernel/event_groups.c
@@ -607,6 +607,10 @@ EventBits_t xEventGroupSetBits( EventGroupHandle_t xEventGroup,
     pxListEnd = listGET_END_MARKER( pxList ); /*lint !e826 !e740 !e9087 The mini list structure is used as the list end to save RAM.  This is checked and valid. */
 #ifdef ESP_PLATFORM // IDF-3755
     taskENTER_CRITICAL();
+    /* The critical section above only takes the event groups spinlock. However, we are about to traverse a task list.
+     * Thus we need call the function below to take the task list spinlock located in tasks.c. Not doing so will risk
+     * the task list's being changed while be are traversing it. */
+    vTaskTakeEventListLock();
 #else
     vTaskSuspendAll();
 #endif // ESP_PLATFORM
@@ -682,6 +686,8 @@ EventBits_t xEventGroupSetBits( EventGroupHandle_t xEventGroup,
         pxEventBits->uxEventBits &= ~uxBitsToClear;
     }
 #ifdef ESP_PLATFORM // IDF-3755
+    /* Release the previously held task list spinlock, then release the event group spinlock. */
+    vTaskReleaseEventListLock();
     taskEXIT_CRITICAL();
 #else
     ( void ) xTaskResumeAll();
@@ -700,6 +706,12 @@ void vEventGroupDelete( EventGroupHandle_t xEventGroup )
 
     // IDF-3755
     taskENTER_CRITICAL();
+#ifdef ESP_PLATFORM
+    /* The critical section above only takes the event groups spinlock. However, we are about to traverse a task list.
+     * Thus we need call the function below to take the task list spinlock located in tasks.c. Not doing so will risk
+     * the task list's being changed while be are traversing it. */
+    vTaskTakeEventListLock();
+#endif
     {
         while( listCURRENT_LIST_LENGTH( pxTasksWaitingForBits ) > ( UBaseType_t ) 0 )
         {
@@ -709,6 +721,10 @@ void vEventGroupDelete( EventGroupHandle_t xEventGroup )
             vTaskRemoveFromUnorderedEventList( pxTasksWaitingForBits->xListEnd.pxNext, eventUNBLOCKED_DUE_TO_BIT_SET );
         }
     }
+#ifdef ESP_PLATFORM
+    /* Release the previously held task list spinlock. */
+    vTaskReleaseEventListLock();
+#endif
     taskEXIT_CRITICAL();
 
         #if ( ( configSUPPORT_DYNAMIC_ALLOCATION == 1 ) && ( configSUPPORT_STATIC_ALLOCATION == 0 ) )
diff --git a/components/freertos/FreeRTOS-Kernel/include/freertos/task.h b/components/freertos/FreeRTOS-Kernel/include/freertos/task.h
index f07774d1b0..0fd3791a88 100644
--- a/components/freertos/FreeRTOS-Kernel/include/freertos/task.h
+++ b/components/freertos/FreeRTOS-Kernel/include/freertos/task.h
@@ -3369,6 +3369,25 @@ void vTaskPlaceOnEventListRestricted( List_t * const pxEventList,
                                       TickType_t xTicksToWait,
                                       const BaseType_t xWaitIndefinitely ) PRIVILEGED_FUNCTION;
 
+#ifdef ESP_PLATFORM
+/*
+ * THIS FUNCTION MUST NOT BE USED FROM APPLICATION CODE.  IT IS AN
+ * INTERFACE WHICH IS FOR THE EXCLUSIVE USE OF THE SCHEDULER.
+ *
+ * This function is a wrapper to take the "xTaskQueueMutex" spinlock of tasks.c.
+ * This lock is taken whenver any of the task lists or event lists are
+ * accessed/modified, such as when adding/removing tasks to/from the delayed
+ * task list or various event lists.
+ *
+ * This functions is meant to be called by xEventGroupSetBits() and
+ * vEventGroupDelete() as both those functions will access event lists (instead
+ * of delegating the entire responsibility to one of vTask...EventList()
+ * functions).
+ */
+void vTaskTakeEventListLock( void );
+void vTaskReleaseEventListLock( void );
+#endif //  ESP_PLATFORM
+
 /*
  * THIS FUNCTION MUST NOT BE USED FROM APPLICATION CODE.  IT IS AN
  * INTERFACE WHICH IS FOR THE EXCLUSIVE USE OF THE SCHEDULER.
diff --git a/components/freertos/FreeRTOS-Kernel/tasks.c b/components/freertos/FreeRTOS-Kernel/tasks.c
index a1998c0a20..66069a681e 100644
--- a/components/freertos/FreeRTOS-Kernel/tasks.c
+++ b/components/freertos/FreeRTOS-Kernel/tasks.c
@@ -3723,6 +3723,20 @@ BaseType_t xTaskRemoveFromEventList( const List_t * const pxEventList )
 }
 /*-----------------------------------------------------------*/
 
+#ifdef ESP_PLATFORM
+void vTaskTakeEventListLock( void )
+{
+    /* We call the tasks.c critical section macro to take xTaskQueueMutex */
+    taskENTER_CRITICAL();
+}
+
+void vTaskReleaseEventListLock( void )
+{
+    /* We call the tasks.c critical section macro to release xTaskQueueMutex */
+    taskEXIT_CRITICAL();
+}
+#endif // ESP_PLATFORM
+
 void vTaskRemoveFromUnorderedEventList( ListItem_t * pxEventListItem,
                                         const TickType_t xItemValue )
 {