buglwIP - A Lightweight TCP/IP stack - Bugs: bug #43278, event_callback() handle context...

 
 

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

bug #43278: event_callback() handle context switch when calling sys_sem_signal()

Submitted by:  Joel Cunningham <jcunningham>
Submitted on:  Mon 22 Sep 2014 02:18:05 PM UTC  
 
Category: sockets/netconnSeverity: 3 - Normal
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: 1.4.1

Tue 28 Oct 2014 09:29:31 PM UTC, comment #1:

Fixed, thanks for reporting.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 22 Sep 2014 02:18:05 PM UTC, original submission:

In event_callback(), the for loop at the bottom that handles signaling waiting application threads that a select is ready, operates under the assumption that the call to sys_sem_signal() won't cause a context switch. The entire loop is protected by a pair of SYS_ARCH_PROTECT() and SYS_ARCH_UNPROTECT().

On certain ports, the _PROTECT and _UNPROTECT calls just disable/enable interrupts. This leaves the possibility of a context switch happening in the call to sys_sem_signal() if the OS primitive preempts the LwIP thread with the waiting task.

The for loop already has support for handling a context switch each iteration and saves the rolling sequence counter in order to detect a context switch when SYS_ARCH_UNPROTECT() is called.

We can address the potential context switch during the call to sys_sem_signal() by moving the line of last_select_cb_ctr = select_ct_ctr to the top of the for loop body, thus ensure any context switch within the loop body is detected. Since we don't modify the select list within the loop body, this is safe

SYS_ARCH_PROTECT(lev);
...
again:
for (scb = select_cb_list; scb != NULL; scb = scb->next) {
if (scb->sem_signalled == 0) {
/* semaphore not signalled yet */
int do_signal = 0;
/* Test this select call for our socket */
if (sock->rcvevent > 0) {
if (scb->readset && FD_ISSET(s, scb->readset)) {
do_signal = 1;
}
}
if (sock->sendevent != 0) {
if (!do_signal && scb->writeset && FD_ISSET(s, scb->writeset)) {
do_signal = 1;
}
}
if (sock->errevent != 0) {
if (!do_signal && scb->exceptset && FD_ISSET(s, scb->exceptset)) {
do_signal = 1;
}
}
if (do_signal) {
scb->sem_signalled = 1;
/* Don't call SYS_ARCH_UNPROTECT() before signaling the semaphore, as this might
lead to the select thread taking itself off the list, invalidagin the semaphore. */
sys_sem_signal(&scb->sem);
}
}
last_select_cb_ctr = select_cb_ctr;
/* unlock interrupts with each step */
SYS_ARCH_UNPROTECT(lev);
/* this makes sure interrupt protection time is short */
SYS_ARCH_PROTECT(lev);
if (last_select_cb_ctr != select_cb_ctr) {
/* someone has changed select_cb_list, restart at the beginning */
goto again;
}

Joel Cunningham <jcunningham>
Project Member

 

Attached Files
file #32167:  event-handle.diff added by jcunningham (691B - application/octet-stream - Diff moves last_select_cb_ctr to top of loop body)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    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 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 28 Oct 2014 09:29:31 PM UTCgoldsimonStatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Mon 22 Sep 2014 02:18:05 PM UTCjcunninghamAttached File-=>Added event-handle.diff, #32167

    Back to the top


    Powered by Savane 3.1-cleanup1