Merge branch 'fix/esp_netif_lock_v5.1' into 'release/v5.1'

fix(esp_netif): Lock netif list with TCPIP context (v5.1)

See merge request espressif/esp-idf!26710

components/esp_netif/esp_netif_objects.c

@@ -1,14 +1,12 @@
 /*
- * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
 
 #include "esp_netif.h"
 #include "sys/queue.h"
 #include "esp_log.h"
-#include "freertos/FreeRTOS.h"
-#include "freertos/semphr.h"
 #include "esp_netif_private.h"
 #include <string.h>
 
@@ -28,65 +26,31 @@ struct slist_netifs_s {
 SLIST_HEAD(slisthead, slist_netifs_s) s_head = { .slh_first = NULL, };
 
 static size_t s_esp_netif_counter = 0;
-static SemaphoreHandle_t  s_list_lock = NULL;
 
 ESP_EVENT_DEFINE_BASE(IP_EVENT);
 
-esp_err_t esp_netif_list_lock(void)
-{
-    if (s_list_lock == NULL) {
-        s_list_lock = xSemaphoreCreateMutex();
-        if (s_list_lock == NULL) {
-            return ESP_ERR_NO_MEM;
-        }
-    }
-    xSemaphoreTake(s_list_lock, portMAX_DELAY);
-    return ESP_OK;
-}
-
-void esp_netif_list_unlock(void)
-{
-    assert(s_list_lock);
-    xSemaphoreGive(s_list_lock);
-    if (s_esp_netif_counter == 0) {
-        vQueueDelete(s_list_lock);
-        s_list_lock = NULL;
-    }
-}
-
 //
 // List manipulation functions
 //
-esp_err_t esp_netif_add_to_list(esp_netif_t *netif)
+esp_err_t esp_netif_add_to_list_unsafe(esp_netif_t *netif)
 {
-    esp_err_t ret;
     struct slist_netifs_s *item = calloc(1, sizeof(struct slist_netifs_s));
-    ESP_LOGD(TAG, "%s %p", __func__, netif);
+    ESP_LOGV(TAG, "%s %p", __func__, netif);
     if (item == NULL) {
         return ESP_ERR_NO_MEM;
     }
     item->netif = netif;
 
-    if ((ret = esp_netif_list_lock()) != ESP_OK) {
-        free(item);
-        return ret;
-    }
-
     SLIST_INSERT_HEAD(&s_head, item, next);
     ++s_esp_netif_counter;
-    ESP_LOGD(TAG, "%s netif added successfully (total netifs: %d)", __func__, s_esp_netif_counter);
-    esp_netif_list_unlock();
+    ESP_LOGD(TAG, "%s netif added successfully (total netifs: %" PRIu32 ")", __func__, (uint32_t)s_esp_netif_counter);
     return ESP_OK;
 }
 
 
-esp_err_t esp_netif_remove_from_list(esp_netif_t *netif)
+esp_err_t esp_netif_remove_from_list_unsafe(esp_netif_t *netif)
 {
     struct slist_netifs_s *item;
-    esp_err_t ret;
-    if ((ret = esp_netif_list_lock()) != ESP_OK) {
-        return ret;
-    }
     ESP_LOGV(TAG, "%s %p", __func__, netif);
 
     SLIST_FOREACH(item, &s_head, next) {
@@ -96,11 +60,9 @@ esp_err_t esp_netif_remove_from_list(esp_netif_t *netif)
             --s_esp_netif_counter;
             ESP_LOGD(TAG, "%s netif successfully removed (total netifs: %d)", __func__, s_esp_netif_counter);
             free(item);
-            esp_netif_list_unlock();
             return ESP_OK;
         }
     }
-    esp_netif_list_unlock();
     return ESP_ERR_NOT_FOUND;
 }
 
@@ -109,17 +71,11 @@ size_t esp_netif_get_nr_of_ifs(void)
     return s_esp_netif_counter;
 }
 
+// This API is inherently unsafe
+// suggest that users call from esp_netif_tcpip_exec()
 esp_netif_t* esp_netif_next(esp_netif_t* netif)
 {
-    esp_err_t ret;
-    esp_netif_t* result;
-    if ((ret = esp_netif_list_lock()) != ESP_OK) {
-        ESP_LOGE(TAG, "Failed to lock esp-netif list with %d", ret);
-        return NULL;
-    }
-    result = esp_netif_next_unsafe(netif);
-    esp_netif_list_unlock();
-    return result;
+    return esp_netif_next_unsafe(netif);
 }
 
 esp_netif_t* esp_netif_next_unsafe(esp_netif_t* netif)
@@ -144,38 +100,23 @@ esp_netif_t* esp_netif_next_unsafe(esp_netif_t* netif)
 bool esp_netif_is_netif_listed(esp_netif_t *esp_netif)
 {
     struct slist_netifs_s *item;
-    esp_err_t ret;
-    if ((ret = esp_netif_list_lock()) != ESP_OK) {
-        ESP_LOGE(TAG, "Failed to lock esp-netif list with %d", ret);
-        return false;
-    }
-
     SLIST_FOREACH(item, &s_head, next) {
         if (item->netif == esp_netif) {
-            esp_netif_list_unlock();
             return true;
         }
     }
-    esp_netif_list_unlock();
     return false;
 }
 
-esp_netif_t *esp_netif_get_handle_from_ifkey(const char *if_key)
+esp_netif_t *esp_netif_get_handle_from_ifkey_unsafe(const char *if_key)
 {
     struct slist_netifs_s *item;
-    esp_err_t ret;
-    if ((ret = esp_netif_list_lock()) != ESP_OK) {
-        ESP_LOGE(TAG, "Failed to lock esp-netif list with %d", ret);
-        return NULL;
-    }
 
     SLIST_FOREACH(item, &s_head, next) {
         esp_netif_t *esp_netif = item->netif;
         if (strcmp(if_key, esp_netif_get_ifkey(esp_netif)) == 0) {
-            esp_netif_list_unlock();
             return esp_netif;
         }
     }
-    esp_netif_list_unlock();
     return NULL;
 }

components/esp_netif/include/esp_netif.h

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2019-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2019-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -975,12 +975,49 @@ int32_t esp_netif_get_event_id(esp_netif_t *esp_netif, esp_netif_ip_event_type_t
 /**
  * @brief Iterates over list of interfaces. Returns first netif if NULL given as parameter
  *
+ * @note This API doesn't lock the list, nor the TCPIP context, as this it's usually required
+ * to get atomic access between iteration steps rather that within a single iteration.
+ * Therefore it is recommended to iterate over the interfaces inside esp_netif_tcpip_exec()
+ *
+ * You can use esp_netif_next_unsafe() directly if all the system
+ * interfaces are under your control and you can safely iterate over them.
+ * Otherwise, iterate over interfaces using esp_netif_tcpip_exec(), or use esp_netif_find_if()
+ * to search in the list of netifs with defined predicate.
+ *
  * @param[in]  esp_netif Handle to esp-netif instance
  *
  * @return First netif from the list if supplied parameter is NULL, next one otherwise
  */
 esp_netif_t *esp_netif_next(esp_netif_t *esp_netif);
 
+/**
+ * @brief Iterates over list of interfaces without list locking. Returns first netif if NULL given as parameter
+ *
+ * Used for bulk search loops within TCPIP context, e.g. using esp_netif_tcpip_exec(), or if we're sure
+ * that the iteration is safe from our application perspective (e.g. no interface is removed between iterations)
+ *
+ * @param[in]  esp_netif Handle to esp-netif instance
+ *
+ * @return First netif from the list if supplied parameter is NULL, next one otherwise
+ */
+esp_netif_t* esp_netif_next_unsafe(esp_netif_t* esp_netif);
+
+/**
+ * @brief Predicate callback for esp_netif_find_if() used to find interface
+ *        which meets defined criteria
+ */
+typedef bool (*esp_netif_find_predicate_t)(esp_netif_t *netif, void *ctx);
+
+/**
+ * @brief Return a netif pointer for the first interface that meets criteria defined
+ * by the callback
+ *
+ * @param fn Predicate function returning true for the desired interface
+ * @param ctx Context pointer passed to the predicate, typically a descriptor to compare with
+ * @return valid netif pointer if found, NULL if not
+ */
+esp_netif_t *esp_netif_find_if(esp_netif_find_predicate_t fn, void *ctx);
+
 /**
  * @brief Returns number of registered esp_netif objects
  *

components/esp_netif/loopback/esp_netif_loopback.c

@@ -187,7 +187,7 @@ esp_netif_t *esp_netif_new(const esp_netif_config_t *esp_netif_config)
     }
     esp_netif->ip_info_old = ip_info;
 
-    esp_netif_add_to_list(esp_netif);
+    esp_netif_add_to_list_unsafe(esp_netif);
 
     // Configure the created object with provided configuration
     esp_err_t ret =  esp_netif_init_configuration(esp_netif, esp_netif_config);
@@ -203,7 +203,7 @@ esp_netif_t *esp_netif_new(const esp_netif_config_t *esp_netif_config)
 void esp_netif_destroy(esp_netif_t *esp_netif)
 {
     if (esp_netif) {
-        esp_netif_remove_from_list(esp_netif);
+        esp_netif_remove_from_list_unsafe(esp_netif);
         free(esp_netif->ip_info);
         free(esp_netif->ip_info_old);
         free(esp_netif->if_key);