[ECOS] net: timeout.c - race conditions

[ECOS] net: timeout.c - race conditions

[Joerg Troeger]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 6872 bytes --]

Some remarks to ecos/timeout.c in the TCP/IP implmentation.

do_timeout() is called in the context of the real-clock-timer DSR.
It calls timeout handlers, which may call the function "timeout()" to
re-enable the timeout (so done for instance by pfslowtimo() and
pffasttimo()).

timeout() also may be called from any other context.

dotimeout() and timeout() both make changes and calculations
on the list of registered timeouts without protecting this list!

1: timeout() may change this list during dotimeout() is working on the list
2: dotimeout() may be called due to an timer interrupt during timeout()
   is working on the list.

In both cases the list may be corrupted (which in fact has been seen happened).

Please have a look to my modified version, which avoids these race conditions
(respect especially "in_dotimeout" and "newflag").

A similar change must be added to untimeout (to be done!).

Jörg Tröger


------- begin modified timeout.c -----------
// ==========================================================================
//
//        lib/timeout.c
//
//        timeout support
//
// ==========================================================================
// ####COPYRIGHTBEGIN####
//                                                                          
// -------------------------------------------                              
// The contents of this file are subject to the Red Hat eCos Public License 
// Version 1.1 (the "License"); you may not use this file except in         
// compliance with the License.  You may obtain a copy of the License at    
// http://www.redhat.com/                                                   
//                                                                          
// Software distributed under the License is distributed on an "AS IS"      
// basis, WITHOUT WARRANTY OF ANY KIND, either express or implied.  See the 
// License for the specific language governing rights and limitations under 
// the License.                                                             
//                                                                          
// The Original Code is eCos - Embedded Configurable Operating System,      
// released September 30, 1998.                                             
//                                                                          
// The Initial Developer of the Original Code is Red Hat.                   
// Portions created by Red Hat are                                          
// Copyright (C) 1998, 1999, 2000 Red Hat, Inc.                             
// All Rights Reserved.                                                     
// -------------------------------------------                              
//                                                                          
// ####COPYRIGHTEND####
// ####BSDCOPYRIGHTBEGIN####
//
// -------------------------------------------
//
// Portions of this software may have been derived from OpenBSD or other sources,
// and are covered by the appropriate copyright disclaimers included herein.
//
// -------------------------------------------
//
// ####BSDCOPYRIGHTEND####
// ==========================================================================
// #####DESCRIPTIONBEGIN####
//
// Author(s):     gthomas
// Contributors:  gthomas
// Date:          1999-02-05
// Description:   Simple timeout functions
// ####DESCRIPTIONEND####

#include <sys/param.h>
#include <pkgconf/net.h>
#include <cyg/kernel/kapi.h>

// Timeout support

#ifndef NTIMEOUTS
#define NTIMEOUTS 8
#endif

typedef struct {
    cyg_int32     delta;  // Number of "ticks" in the future for this timeout
    timeout_fun  *fun;    // Function to execute when it expires
    void         *arg;    // Argument to pass when it does
 int    newflag;
} timeout_entry;
static timeout_entry timeouts[NTIMEOUTS];
static cyg_handle_t timeout_alarm_handle;
static cyg_alarm timeout_alarm;
static cyg_int32 last_delta;
static int in_dotimeout = 0;

static void
do_timeout(cyg_handle_t alarm, cyg_addrword_t data)
{
    int i;
    cyg_int32 min_delta;
    timeout_entry *e = timeouts;

 in_dotimeout = 1;

    for (i = 0;  i < NTIMEOUTS;  i++, e++) {

  // don_t respect entries added new in timeout handlers!
  if (e->newflag)
   continue;

        if (e->delta) {
            e->delta -= last_delta;
            if (e->delta == 0) {
                // Time for this item to 'fire'
                (e->fun)(e->arg);
                e->fun = 0;
            }
        }
    }

 // activate all new added entries
    for (i=0, e=timeouts;  i < NTIMEOUTS;  i++, e++) {
  if (e->newflag)
   e->newflag = 0;
 }

    min_delta = 0x7FFFFFFF;
    for (i=0, e=timeouts;  i < NTIMEOUTS;  i++, e++) {
        if (e->delta && (e->delta < min_delta)) min_delta = e->delta;
    }
    if (min_delta != 0x7FFFFFFF) {
        // Still something to do, schedule it
        cyg_alarm_initialize(timeout_alarm_handle, cyg_current_time()+min_delta, 0);
        last_delta = min_delta;
    }

 in_dotimeout = 0;
}

cyg_uint32
timeout(timeout_fun *fun, void *arg, cyg_int32 delta)
{
    int i;
    cyg_int32 min_delta;
    static bool init = false;
    timeout_entry *e = timeouts;
    cyg_uint32 stamp;

    if (!init) {
        cyg_handle_t h;
        cyg_clock_to_counter(cyg_real_time_clock(), &h);
        cyg_alarm_create(h, do_timeout, 0, &timeout_alarm_handle, &timeout_alarm);
        init = true;
    }
    stamp = 0;  // Assume no slots available

 // assure do_timeout() is not called within this block
 cyg_scheduler_lock();

    for (i = 0;  i < NTIMEOUTS;  i++, e++) {
        if ((e->delta == 0) && (e->fun == 0)) {
            // Free entry
   e->newflag = in_dotimeout;
            e->delta = delta;
            e->fun = fun;
            e->arg = arg;
            stamp = (cyg_uint32)e;
            break;
        }
    }
 if (!in_dotimeout) {
     min_delta = 0x7FFFFFFF;
     for (i=0, e=timeouts;  i < NTIMEOUTS;  i++, e++) {
         if (e->delta && (e->delta < min_delta)) min_delta = e->delta;
     }
     if (min_delta != 0x7FFFFFFF) {
         // Still something to do, schedule it
         cyg_alarm_initialize(timeout_alarm_handle, cyg_current_time()+min_delta, 0);
         last_delta = min_delta;
     }
 }

 cyg_scheduler_unlock();
 // block protected against DSRs

    return stamp;
}

void
untimeout(timeout_fun *fun, void * arg)
{
  int i;
  timeout_entry *e = timeouts;

  for (i = 0; i < NTIMEOUTS; i++, e++) {
    if (e->delta && (e->fun == fun) && (e->arg == arg)) {
            e->delta = 0;
            e->fun = 0;
      return;
        }
    }
}

------- end modified timeout.c -----------

Re: [ECOS] net: timeout.c - race conditions

[Andrew Lunn]

Hi Joerg

I've been having discussions about these issues with RedHat as
well. There is work in progress to fix this. I don't now the current
status though. Maybe Redhat can comment.

        Andrew

Re: [ECOS] net: timeout.c - race conditions

[Hugo Tyson]

andrew.lunn@ascom.ch (Andrew Lunn) writes:
> Hi Joerg
> 
> I've been having discussions about these issues with RedHat as
> well. There is work in progress to fix this. I don't now the current
> status though. Maybe Redhat can comment.

I'm testing a fixed version even as I type.

It will be available in the next anoncvs update, I think, BUT the problem
is it requires a subtle change in kernel thread-sleep semantics (which we
had implemented for other reasons by the by) that let me make a far neater
solution than would otherwise have been possible.

So I can't just publish a patch or even a couple of new files ;-( it's the
whole thing.

Being unwilling to read Joerg's messages because they are not plain text,
I can't comment more specifically.  Turn off base-64 encoding please Joerg.

	- Huge