buglwIP - A Lightweight TCP/IP stack - Bugs: bug #27996, Thread-safety of tcp_timer_needed()

 
 

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

bug #27996: Thread-safety of tcp_timer_needed()

Submitter:  Albert Bartel <albertb>
Submitted:  Wed 11 Nov 2009 01:53:00 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Invalid
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Wed 11 Nov 2009 03:58:35 PM UTC, comment #5: 


> You say, that if it is called from a different thread that's the bug.
> So is it wrong to call ethernet_input() from the separate ethernetif thread?


Yes, in two ways:

1) ethernet_input() (as any other function from the core files - except for some pbuf/mem/memp functions) must not be called from any other thread than the tcpip_thread

2) the port should not call ethernet_input() but instead initialize the netif correctly with the netif_add() call, set the flag NETIF_FLAG_ETHARP in its _init() function (see example ethernetif.c) and then call netif->input() instead of ethernet_input() (also, see example ethernetif.c), which takes care of getting into the correct thread and caling ethernet_input().

As you see, the example ethernetif.c would have been a good guidance :-)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 11 Nov 2009 03:27:48 PM UTC, comment #4: 

Something should be calling tcpip_input() instead of ip_input() directly.  That is most likely your bug.

Kieran Mansley <kieranm>
Group Member
Wed 11 Nov 2009 03:17:33 PM UTC, comment #3: 

I have got an ATMEL AVR32 UC3 port for FreeRTOS. In this port the ethernetif.c creates a thread for ethernetif_input.
The thread function is:
static void ethernetif_input(void * pvParameters)
{
  struct netif      netif = (struct netif )pvParameters;
  struct pbuf       *p;

  for( ;; )
  {
    do
    {
      /* move received packet into a new pbuf */
      p = low_level_input( netif );
      if( p == NULL )
      {
        /* No packet could be read.  Wait a for an interrupt to  
           tell us there is more data available. */
        vMACBWaitForInput(100);
      }
    }while( p == NULL );

    if( ERR_OK != ethernet_input( p, netif ) )
    {
      pbuf_free(p);
      p = NULL;
    }
  }
}

In this port, if a SYN arrives, ethernet_input(struct pbuf *p, struct netif *netif) is called.
Then ip_input(struct pbuf *p, struct netif *inp) is called,
then tcp_input(struct pbuf *p, struct netif *inp),
then tcp_listen_input(struct tcp_pcb_listen *pcb)
and there TCP_REG((pcbs, npcb)) is called,
that calls tcp_timer_needed().
So the timeout will be set only for this thread.
When I set a breakpoint in the tcpip_tcp_timer-function, I never get there, because the tcpip thread does not handle the timeout, because it is in the wrong thread.
After changing sys_timeout() to tcpip_timeout() I pass the breakpoint if a timeout occurrs.

You say, that if it is called from a different thread that's the bug. So is it wrong to call ethernet_input() from the separate ethernetif thread?

Albert Bartel <albertb>
Wed 11 Nov 2009 02:05:19 PM UTC, comment #2: 

I don't think this is necessary because tcp_timer_needed() is only called from tcpip_thread. If not, that's the bug.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 11 Nov 2009 01:56:07 PM UTC, comment #1: 

Can you give a bit more detail about the problem you're seeing that this change fixes?


Kieran Mansley <kieranm>
Group Member
Wed 11 Nov 2009 01:53:00 PM UTC, original submission:  

The function tcp_timer_needed(void) should not call

sys_timeout(TCP_TMR_INTERVAL, tcpip_tcp_timer, NULL);

directly, but tell the tcpip thread to set the timeout thread-safe by calling the existing function

tcpip_timeout(TCP_TMR_INTERVAL, tcpip_tcp_timer, NULL);

that sends TCPIP_MSG_TIMEOUT to the tcpip thread.

Albert Bartel <albertb>

 

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

No files currently attached

 

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 kieranm (Posted a comment)
  • -email is unavailable- added by albertb (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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-11-11 goldsimon StatusNone Invalid
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code