buglwIP - A Lightweight TCP/IP stack - Bugs: bug #46321, Synchronization bug around...

 
 

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

bug #46321: Synchronization bug around lwip_select() and tcpip_thread() with thread-local semaphores

Submitted by:  Mikhail Pankov <pankov_m>
Submitted on:  Thu 29 Oct 2015 10:07:14 AM UTC  
 
Category: sockets/netconnSeverity: 3 - Normal
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: git head

(Jump to the original submission Jump to the original submission)

Mon 16 Nov 2015 08:45:10 AM UTC, comment #6:

For me, this is done.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 02 Nov 2015 12:09:01 PM UTC, comment #5:

Re comment #3

> I second the sys_arch_sem_trywait() idea!


I don't think we need this: the select_cb struct has a member to ensure its semaphore is only signalled once. That way, we should never block at the point where I used the 1ms timeout since we only wait if the semaphore is signalled and we didn't get this signal.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 02 Nov 2015 11:36:41 AM UTC, comment #4:

Thank you for a prompt fix! I'll get back in touch once I have the time to check if it works in our project.

Mikhail Pankov <pankov_m>
Thu 29 Oct 2015 10:36:26 PM UTC, comment #3:

I second the sys_arch_sem_trywait() idea! :)

The millisecond wait may reduce performance in some high frequency applications that call select and encounter this timing issue

Joel Cunningham <jcunningham>
Project Member
Thu 29 Oct 2015 09:21:35 PM UTC, comment #2:

I've pushed a fix which should fix this. Could you try if it works for you?

Thanks for reporting and thanks for the detailed report!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 29 Oct 2015 09:09:32 PM UTC, comment #1:

Wow, that's one long post!

The best way to fix this would be a call to 'sys_archm_sem_trywait()', but unfortunately, this function doesn't exist (yet). I'm tempted to introduce it, though...

Another option would be to wait one milisecond after taking select_cb off the list (unless we already waited and got signalled)...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
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.

Mikhail Pankov <pankov_m>

 

Attached Files
file #35329:  lwip-assert-tls-sem.patch added by pankov_m (3KiB - text/x-patch - patch to work around the problem)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jcunningham (Posted a comment)
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by pankov_m (Submitted the item)
  • -unavailable- added by pankov_m
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 16 Nov 2015 08:45:10 AM UTCgoldsimonStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Thu 29 Oct 2015 09:21:35 PM UTCgoldsimonStatusNone=>Ready For Test
      Assigned toNone=>goldsimon
    Thu 29 Oct 2015 10:40:17 AM UTCpankov_mCarbon-CopyRemoved pankov_m=>-
    Thu 29 Oct 2015 10:07:14 AM UTCpankov_mAttached File-=>Added lwip-assert-tls-sem.patch, #35329
      Carbon-Copy-=>Added pankov_m
      Carbon-Copy-=>Added -unavailable-

    Back to the top


    Powered by Savane 3.1-cleanup1