buglwIP - A Lightweight TCP/IP stack - Bugs: bug #65786, tcpip_init fails...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #65786: tcpip_init fails LWIP_ASSERT_CORE_LOCKED assertion

Submitter:  Godmar Back <godmar>
Submitted:  Fri 24 May 2024 01:49:59 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Crash Error Status:  Works For Me
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  2.2.0

Thu 04 Jul 2024 02:03:58 PM UTC, comment #5: 

Closing this as invalid. It's like it is by design!

Simon Goldschmidt <goldsimon>
Group administrator
Fri 31 May 2024 05:45:05 PM UTC, comment #4: 

I think there is a reason for calling lwip_init() first. lwip_init() also calls sys_init() and before that, you have no guarantee that the sys_* functions can be called at all. So calling  sys_mutex_new() before lwip_init() only works for ports which don't do much in sys_init(). If the system's mutex creation requires any special initialization in sys_init(), it won't work.

You mentioned that your failure scenario can be prevented by using the callback function to rendezvous with the TCP/IP thread. Note that this is exactly what is being done in the example_app (contrib). So I would say it's a pretty common pattern.

Finally, in a multithreaded environment, it might be wiser to call netifapi_netif_add() instead of the "raw" netif_add() by enabling LWIP_NETIF_API. It won't clear all the possible concurrent access scenarios, but at least the netif-related functions should be handled safely.

Matthias Dietrich <matthiasdietrich>
Fri 31 May 2024 01:28:08 PM UTC, comment #3: 

On another note, I'm looking at how I missed this.

I started my lwipopts.h based on the example here:

https://git.savannah.nongnu.org/cgit/lwip.git/tree/contrib/examples/example_app/lwipopts.h#n315

This mentions that LWIP_ASSERT_CORE_LOCKED is required, but it doesn't mention LWIP_MARK_TCPIP_THREAD at all.

Godmar Back <godmar>
Fri 31 May 2024 01:02:34 PM UTC, comment #2: 

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 ).

Godmar Back <godmar>
Thu 30 May 2024 05:15:41 PM UTC, comment #1: 

Take a look at some of the existing ports:
https://git.savannah.nongnu.org/cgit/lwip.git/tree/contrib/ports/freertos/sys_arch.c#n582
https://git.savannah.nongnu.org/cgit/lwip.git/tree/contrib/ports/unix/port/sys_arch.c#n246
https://git.savannah.nongnu.org/cgit/lwip.git/tree/contrib/ports/win32/sys_arch.c#n507

In all of them LWIP_ASSERT_CORE_LOCKED (sys_check_core_locking) does not
check the mutex or thread identity (depending on the value of
LWIP_TCPIP_CORE_LOCKING) unless the TCPIP thread has started and called
LWIP_MARK_TCPIP_THREAD (sys_mark_tcpip_thread).

Billy Bednar <bb1>
Fri 24 May 2024 01:49:59 PM UTC, original submission:  

I'm trying to use lwip 2.2-STABLE with

#define LWIP_TCPIP_CORE_LOCKING    1

and have implemented the necessary sys_arch functions.

Calling tcpip_init leads to a sequence where the startup sequence fails since the code asserts that the TCP core lock is held before it was even created.

Specifically, tcpip_init calls lwip_init, which calls netif_init, which calls netif_add for the loopback interface, which asserts LWIP_ASSERT_CORE_LOCKED().

See https://lists.nongnu.org/archive/html/lwip-users/2024-05/msg00002.html

Possible fix: move lines 668ff before lwip_init, see https://github.com/lwip-tcpip/lwip/blob/86c9f7999150199374d7697fd2eed62dcd9b1afa/src/api/tcpip.c#L668-L672

I'm happy to submit a PR if needed (on Github?)


Godmar Back <godmar>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by matthiasdietrich (Posted a comment)
  • -email is unavailable- added by bb1 (Posted a comment)
  • -email is unavailable- added by godmar (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-07-04 goldsimon StatusNone Works For Me
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-b921.
    Corresponding source code