Thu 29 Oct 2015 10:07:14 AM UTC, original submission:
We've hit a pretty obscure synchronization bug in LwIP.
The summary follows.
lwip_select() may leave thread-local semaphore in inconsistent state if new packets arrive between the 2 calls to lwip_selscan() in the function. Thread switch may happen in the meantime and semaphore will be signaled before it's being waited on. Moreover, it will never be waited on as second lwip_selscan() will detect new packets and choose not to wait on semaphore at all. This same semaphore is then used in many other places, for example, in tcpip_apimsg. That causes corruption of stack-allocated API message. This happens because the user thread calling the LwIP thread that does tcpip_thread() should wait on that same thread-local semaphore, but won't, as it's already signaled.
The observed behavior is with LWIP_NETCONN_SEM_PER_THREAD defined.
We have actually observed the crash resulting from described behavior. It depends on scheduling of threads a lot, but it did happen after hours of work in multi-threaded environment. When I write "we switch from thread 1 to thread 2" below, I mean it happens sometimes, very rarely actually.
Please bear with me as detailed description is non-trivial.
Let's look at lwip_select:
With LWIP_NETCONN_SEM_PER_THREAD defined, we use thread-local semaphores.
Now, consider a situation. We performed the selscan in the beginning of the function:
and added ourselves to the list of waiting threads.
Then, while we are checking the validity of FDSETs
we receieve the packet and that causes a thread switch to the thread doing event_callback().
We are now on the list, and the packet is for the socket that's being waited on by user's select(). We go ahead and signal the semaphore:
Then we switch back to thread in lwip_select(), that had nothing ready (thus nready == 0):
We do lwip_selscan again and discover we have packets for our socket that arrived after the previous scan and before this one. Our semaphore is already signalled!
Now we get nready > 0 and don't go in this block:
So we don't wait on the semaphore in this select() call and leave. The semaphore, being thread-local, is left signaled. When we enter any other function from user thread, that has to wait on thread-local semaphore, it's already signaled, and thread continues immediately, causing havoc.
In our particular example, the next function user thread happened to enter was tcpip_apimsg().
As you can see, we fetch thread-local semaphore
that is already signaled, as you remember.
Then the thread posts the mbox and, instead of waiting, immediately continues and exits the function, thus trashing stack-allocated msg.
The thread that processes the mbox tries to dispatch the message
and falls through as type is garbage
So, this sequence of events leads to hard crash of LwIP due to assert.
We have worked around this problem by not using thread-local semaphore in lwip_select() and using usual function-local stack semaphore instead. See the below patch (it's also attached):
diff --git a/contrib/src/api/sockets.c b/contrib/src/api/sockets.c
index 165fbba..f595fad 100644
--- a/contrib/src/api/sockets.c
+++ b/contrib/src/api/sockets.c
@@ -211,13 +211,13 @@ struct lwip_sock {
SELWAIT_T select_waiting;
};
-#if LWIP_NETCONN_SEM_PER_THREAD
-#define SELECT_SEM_T sys_sem_t*
-#define SELECT_SEM_PTR(sem) (sem)
-#else /* LWIP_NETCONN_SEM_PER_THREAD */
#define SELECT_SEM_T sys_sem_t
#define SELECT_SEM_PTR(sem) (&(sem))
-#endif /* LWIP_NETCONN_SEM_PER_THREAD */
/** Description for a task waiting in select */
struct lwip_select_cb {
@@ -1226,15 +1226,15 @@ lwip_select(int maxfdp1, fd_set readset, fd_set writeset, fd_set *exceptset,
select_cb.writeset = writeset;
select_cb.exceptset = exceptset;
select_cb.sem_signalled = 0;
-#if LWIP_NETCONN_SEM_PER_THREAD
- select_cb.sem = LWIP_NETCONN_THREAD_SEM_GET();
-#else /* LWIP_NETCONN_SEM_PER_THREAD */
if (sys_sem_new(&select_cb.sem, 0) != ERR_OK) {
/* failed to create semaphore */
set_errno(ENOMEM);
return -1;
}
-#endif /* LWIP_NETCONN_SEM_PER_THREAD */
/* Protect the select_cb_list */
SYS_ARCH_PROTECT(lev);
@@ -1334,9 +1334,9 @@ lwip_select(int maxfdp1, fd_set readset, fd_set writeset, fd_set *exceptset,
select_cb_ctr++;
SYS_ARCH_UNPROTECT(lev);
-#if !LWIP_NETCONN_SEM_PER_THREAD
sys_sem_free(&select_cb.sem);
-#endif /* LWIP_NETCONN_SEM_PER_THREAD */
if (nready < 0) {
/* This happens when a socket got closed while waiting */
With these changes, LwIP works for us. Whether this is a proper fix is still questionable, however.
|