[lwip-devel] [bug #65786] tcpip_init fails LWIP_ASSERT_CORE_LOCKED assertion

[Godmar Back]

Follow-up Comment #2, bug #65786 (group lwip):

Thank you, I see now how skipping this check can prevent the failure I'm
seeing.

I do not believe that this approach is correct, however. It could potentially
fail in two ways.

First, by skipping the check until LWIP_MARK_TCPIP_THREAD is called, the
following sequence of events can occur:
- Thread A calls tcpip_init
- Thread A calls a function such as netif_add, which skips the check
- Thread A is now in a critical section, but doesn't hold the lock
- TCP/IP thread starts, can acquire the lock successfully, and races with
thread A

(Note that this scenario can be prevented if Thread A uses the init callback
function to rendezvous with the TCP/IP thread, say by using semaphore until it
is called.  The code should assert that the callback function is not NULL, at
least when core locking is enabled, but it currently allows this.)

Second, it diminishes the usefulness of the LWIP_ASSERT_CORE_LOCKED assertion
in the presence of other threads.

If the idea is that this assertion ensures that shared data structures are
accessed in a way that is free from data races, then it must be asserted
always, not only when the TCP/IP thread is running.  Otherwise, it will only
be able to detect data races between then TCP/IP threads and others, but not
data races that can arise when multiple non-TCP/IP threads call functions that
access shared data that must be protected.

As a side note, I seem to be able to run successfully (with an active
LWIP_ASSERT_CORE_LOCKED, but with an empty LWIP_MARK_TCPIP_THREAD) by moving
the creation of the lock up, as in:

diff -ur /home/staff/gback/pintos/lwip-STABLE-2_2_0_RELEASE/src/api/tcpip.c
./api/tcpip.c
---
/home/staff/gback/pintos/lwip-STABLE-2_2_0_RELEASE/src/api/tcpip.c      
2023-09-25
15:25:35.000000000 -0400
+++ ./api/tcpip.c       2024-05-24 09:52:43.261045072 -0400
@@ -649,18 +649,23 @@
 void
 tcpip_init(tcpip_init_done_fn initfunc, void *arg)
 {
+#if LWIP_TCPIP_CORE_LOCKING
+  if (sys_mutex_new(&lock_tcpip_core) != ERR_OK) {
+    LWIP_ASSERT("failed to create lock_tcpip_core", 0);
+  }
+  LOCK_TCPIP_CORE();
+#endif /* LWIP_TCPIP_CORE_LOCKING */
+
   lwip_init();
+#if LWIP_TCPIP_CORE_LOCKING
+  UNLOCK_TCPIP_CORE();
+#endif /* LWIP_TCPIP_CORE_LOCKING */
 
   tcpip_init_done = initfunc;
   tcpip_init_done_arg = arg;
   if (sys_mbox_new(&tcpip_mbox, TCPIP_MBOX_SIZE) != ERR_OK) {
     LWIP_ASSERT("failed to create tcpip_thread mbox", 0);
   }
-#if LWIP_TCPIP_CORE_LOCKING
-  if (sys_mutex_new(&lock_tcpip_core) != ERR_OK) {
-    LWIP_ASSERT("failed to create lock_tcpip_core", 0);
-  }
-#endif /* LWIP_TCPIP_CORE_LOCKING */
 
   sys_thread_new(TCPIP_THREAD_NAME, tcpip_thread, NULL,
TCPIP_THREAD_STACKSIZE, TCPIP_THREAD_PRIO);
 }

Perhaps this could alleviate the need for marking the TCP/IP thread at all?

This may decrease the burden on ports. On a semi-related note, looking at the
Unix port, the current implementation in the "unix" port directly compares
pthread_t rather than using pthread_equal, and it assumes that they can be
compared to 0, which POSIX asks to not assume (see
https://www.man7.org/linux/man-pages/man3/pthread_self.3.html#NOTES ). 



    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?65786>

_______________________________________________
Message sent via Savannah
https://savannah.nongnu.org/