fix(esp_netif): Fix races in netif object locking

components/esp_netif/esp_netif_objects.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -32,25 +32,41 @@ static SemaphoreHandle_t  s_list_lock = NULL;
 
 ESP_EVENT_DEFINE_BASE(IP_EVENT);
 
-esp_err_t esp_netif_list_lock(void)
+esp_err_t esp_netif_objects_init(void)
 {
-    if (s_list_lock == NULL) {
+    if (s_list_lock != NULL) {
+        // already initialized
+        return ESP_OK;
+    }
     s_list_lock = xSemaphoreCreateMutex();
     if (s_list_lock == NULL) {
         return ESP_ERR_NO_MEM;
     }
-    }
+    return ESP_OK;
+}
+
+void esp_netif_objects_deinit(void)
+{
+    vSemaphoreDelete(s_list_lock);
+    s_list_lock = NULL;
+}
+
+esp_err_t esp_netif_list_lock(void)
+{
+    if (s_list_lock) {
         xSemaphoreTake(s_list_lock, portMAX_DELAY);
+    } else {
+        ESP_LOGD(TAG, "%s list not locked (s_list_lock not initialized)", __func__);
+    }
     return ESP_OK;
 }
 
 void esp_netif_list_unlock(void)
 {
-    assert(s_list_lock);
+    if (s_list_lock) {
         xSemaphoreGive(s_list_lock);
-    if (s_esp_netif_counter == 0) {
+    } else {
-        vQueueDelete(s_list_lock);
+        ESP_LOGD(TAG, "%s list not unlocked (s_list_lock not initialized)", __func__);
-        s_list_lock = NULL;
     }
 }
 

components/esp_netif/lwip/esp_netif_lwip.c

@@ -148,14 +148,6 @@ static esp_err_t set_lwip_netif_callback(struct esp_netif_api_msg_s *msg)
     return ESP_OK;
 }
 
-static esp_err_t remove_lwip_netif_callback(struct esp_netif_api_msg_s *msg)
-{
-    (void)msg;
-    netif_remove_ext_callback(&netif_callback);
-    memset(&netif_callback, 0, sizeof(netif_callback));
-    return ESP_OK;
-}
-
 static void dns_clear_servers(bool keep_fallback)
 {
     u8_t numdns = 0;
@@ -502,6 +494,10 @@ static void tcpip_init_done(void *arg)
 
 esp_err_t esp_netif_init(void)
 {
+    if (esp_netif_objects_init() != ESP_OK) {
+        ESP_LOGE(TAG, "esp_netif_objects_init() failed");
+        return ESP_FAIL;
+    }
     if (!sys_thread_tcpip(LWIP_CORE_IS_TCPIP_INITIALIZED)) {
 #if CONFIG_LWIP_HOOK_TCP_ISN_DEFAULT
         uint8_t rand_buf[16];
@@ -559,6 +555,11 @@ esp_err_t esp_netif_init(void)
 
 esp_err_t esp_netif_deinit(void)
 {
+    /* esp_netif_deinit() is not supported (as lwIP deinit isn't suported either)
+     * Once it's supported, we need to de-initialize:
+     * - netif objects calling esp_netif_objects_deinit()
+     * - other lwIP specific objects (see the comment after tcpip_initialized)
+     */
     if (sys_thread_tcpip(LWIP_CORE_IS_TCPIP_INITIALIZED)) {
         /* deinit of LwIP not supported:
          * do not deinit semaphores and states,
@@ -566,6 +567,7 @@ esp_err_t esp_netif_deinit(void)
          *
         sys_sem_free(&api_sync_sem);
         sys_sem_free(&api_lock_sem);
+        netif_remove_ext_callback(); (in lwip context)
          */
         return ESP_ERR_NOT_SUPPORTED;
 
@@ -736,8 +738,6 @@ esp_netif_t *esp_netif_new(const esp_netif_config_t *esp_netif_config)
 
     esp_netif->lwip_netif = lwip_netif;
 
-    esp_netif_add_to_list(esp_netif);
-
 #if ESP_DHCPS
     // Create DHCP server structure
     if (esp_netif_config->base->flags & ESP_NETIF_DHCP_SERVER) {
@@ -763,6 +763,8 @@ esp_netif_t *esp_netif_new(const esp_netif_config_t *esp_netif_config)
         esp_netif_lwip_ipc_no_args(set_lwip_netif_callback);
     }
 
+    esp_netif_add_to_list(esp_netif);
+
     return esp_netif;
 }
 
@@ -858,9 +860,11 @@ void esp_netif_destroy(esp_netif_t *esp_netif)
 {
     if (esp_netif) {
         esp_netif_remove_from_list(esp_netif);
-        if (esp_netif_get_nr_of_ifs() == 0) {
+        // not calling  `netif_remove_ext_callback()` if number of netifs is 0
-            esp_netif_lwip_ipc_no_args(remove_lwip_netif_callback);
+        // since it's unsafe.
-        }
+        // It is expected to be called globally in `esp_netif_deinit()`
+        // once it's supported.
+        // }
         free(esp_netif->ip_info);
         free(esp_netif->ip_info_old);
         free(esp_netif->if_key);

components/esp_netif/private_include/esp_netif_private.h

@@ -165,4 +165,17 @@ esp_err_t esp_netif_add_ip6_address(esp_netif_t *esp_netif, const ip_event_add_i
  */
 esp_err_t esp_netif_remove_ip6_address(esp_netif_t *esp_netif, const esp_ip6_addr_t *addr);
 
+/**
+ * @brief Initialize netif objects for handling lists of interfaces one esp_netif level
+ *
+ * @return esp_err_t ESP_OK on success
+ */
+esp_err_t esp_netif_objects_init(void);
+
+/**
+ * @brief Deinitialize netif objects
+ *
+ */
+void esp_netif_objects_deinit(void);
+
 #endif //_ESP_NETIF_PRIVATE_H_