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

Submitter:  Joel Cunningham <jcunningham>
Submitted:  Mon 22 Sep 2014 02:18:05 PM UTC
   
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned 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>
Group administrator
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>
Group Member

 

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

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
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jcunningham (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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-10-28 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2014-09-22 jcunningham Attached File- Added event-handle.diff, #32167

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code