buglwIP - A Lightweight TCP/IP stack - Bugs: bug #23240, recv_udp increases counters for...

 
 

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

bug #23240: recv_udp increases counters for available receives before netbuf is actually posted

Submitted by:  Steinar Lieng Fredriksen <steinarlf>
Submitted on:  Tue 13 May 2008 10:51:07 AM UTC  
 
Category:  UDP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.0

( Jump to the original submission)

Mon 06 Jul 2009 03:57:22 PM UTC, comment #16: 

Seems like a good thing to do, yes.

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 06 Jul 2009 02:28:32 PM UTC, comment #15: 

I thought about that and decided that as the user was probably expecting an unsigned value, it was better to cast it to unsigned.

What I will do instead is retrieve the value of recv_avail, round it up to zero if it is negative, and return that as an unsigned value.  I think that should make sense in the circumstances.

Kieran Mansley <kieranm>
Project Member
Mon 06 Jul 2009 09:53:28 AM UTC, comment #14: 

I guess the patch looks quite good except for the usage of conn->recv_avail in lwip_ioctl() case FIONREAD: it is casted to u16_t, which should be converted to s16_t, too.

Except for that, I guess it's OK as a fix, but I'm not in a position to actually test it.

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 26 Jun 2009 02:44:41 PM UTC, comment #13: 

I've attached a patch which I think might address this in the short term, using Jifl's suggestion of making the variables signed and adjusting the tests of those variables to consider anything < 0 as equivalent to zero.  It also moves the SYS_ARCH_INC and API_EVENT calls to only occur if the sys_mbox_trypost() call is successful.  I have done this for the recv_udp, recv_tcp, recv_raw and accept_function functions in api_msg.c as they all seemed to have the same bit of code.

Before I check this in I would welcome some code review from the other developers as I want to check that I have correctly understood their proposed solution.

If anyone can test that this actually works that would be great too.

(file #18330)

Kieran Mansley <kieranm>
Project Member
Thu 25 Jun 2009 09:59:42 AM UTC, comment #12: 

This is now the last bug that needs fixing before 1.3.1, so I would welcome any opinions as soon as possible!

Kieran Mansley <kieranm>
Project Member
Wed 24 Jun 2009 03:24:09 PM UTC, comment #11: 

comment #10 second paragraph should read:

"While making rcvevent and recv_avail signed might just about end up with the right functionality..."

sorry!

Kieran Mansley <kieranm>
Project Member
Wed 24 Jun 2009 03:16:11 PM UTC, comment #10: 

I've been looking at this and don't really like any of the proposed solutions.

While the making rcvevent and recv_avail might just about end up with the right functionality, I don't like that between doing the trypost and incrementing recv_avail the state would be in an inconsistent state, and this could be so for some time if the trypost causes another thread to execute.

My preferred way of doing it would be to add to the mbox API and have a sys_mbox_tryreserve() which would see if there was a free slot in the mbox, and if so leave the mbox locked so that it couldn't be taken by another thread.  If this returned true you'd know that the post operation was going to be successful, you could increment the counters in a sensible order, then do a sys_mbox_postreserved() which would post the message and unlock the mailbox.  If tryreserve() returned false you'd just give up at that point without having to undo any state changes you'd made already:

if(sys_mbox_tryreserve() {
  /* Slot in mbox is reserved, mbox is left locked */

  SYS_ARCH_INC(conn->recv_avail, p->tot_len);
  /* Register event with callback */
  API_EVENT(conn, NETCONN_EVT_RCVPLUS, p->tot_len);
  sys_mbox_postreserved();
} else {
  /* Slot in mbox not available, mbox is not left locked */

  netbuf_delete(buf);
  return;
}

The only problem I can see with this is (i) it requires a change to the API, so ports need updating and (ii) if API_EVENT tried to access the mbox it would find it was locked, which could lead to deadlock.  I'm not sure if this is likely or a problem.  I guess it is the same as using a semaphore as proposed in comment #3 but using the mbox protection instead of a new semaphore.

Kieran Mansley <kieranm>
Project Member
Mon 11 May 2009 08:00:57 AM UTC, comment #9: 

hi, all.
I'm a new comer of lwip. I also have this bug in our embedded system, so I trace the code.

I'm wondering whether using a buf linked-list for each netconn help. sys_mbox_trypost() is just used for synchronization, not for passing buf pointer. Since if sys_mbox_trypost() fails, the code has to free the buf(netbuf_delete()), and this wastes CPU resource (we copy the data from driver, but have no chance to process it). 

Besides, in my opinion, sys_mbox_trypost() fail probably results from mbox full, so it might be good to keep the buf.

How do you think?
(Sorry for my bad English.)

Daphne <likafann>
Tue 21 Apr 2009 08:18:03 PM UTC, comment #8: 

Jonathan, I do think converting the counters to signed would work. However, after working with lwIP for over 3 years now, I still don't like the fact that we need three (!) counters here (recvmbox will have a counter, recv_avail and API_EVENT).

But since there have been some deadlock reports on the lwip-lists in the last few months, I guess solving this neatly can be left to sockets2 and converting the counters to signed should do the trick.

D'you have time to implement and test that, or do I have to jump in? g

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 30 Mar 2009 12:15:16 PM UTC, comment #7: 

A reminder: bug #25821, which was just closed as a duplicate of this bug, addressed the same issue but in recv_tcp instead of recv_udp.

Wim Dumon <wdumon>
Mon 30 Mar 2009 11:55:27 AM UTC, comment #6: 

I'd really like to see some progress on this bug before the next release.  Any chance one of the developers could take a look at it?  Jifl - you seem to have given this most thought so far.

Kieran Mansley <kieranm>
Project Member
Mon 09 Jun 2008 12:47:52 PM UTC, comment #5: 

I haven't had a chance to test this any further yet, will hand in my thesis tomorrow.

On a general note, if a variable is to be shared by several threads without some OS level protection, it must be ensured that the read and write operations are atomic. 

Steinar Lieng Fredriksen <steinarlf>
Mon 02 Jun 2008 11:50:41 AM UTC, comment #4: 

Not sure about that. This would have to also cover access to sock->rcvevent. Although I haven't thought through in detail it seems to me there's a risk of deadlock (semaphores not being like recursive mutexes).

I think we just need to make these counts be signed quantities (as well as the proposed fix in comment #0), to avoid problems if they wrap. I'd just like another opinion on whether that idea has any flaws. Even if a semaphore wouldn't risk deadlocks, it may be overkill.

Jonathan Larmour <jifl>
Project Member
Sat 31 May 2008 05:21:06 PM UTC, comment #3: 

I guess what we want to achieve is consistency between the count and the mailbox, so isn't it better to use a semaphore to ensure that either all or none of the operations are carried out?

As far as I can tell there are no potentially blocking calls within the protected regions, so something like this could work:

wait recv_count_sema;
if (sys_mbox_trypost(conn->recvmbox, buf) != ERR_OK) {
netbuf_delete(buf);
signal recv_count_sema;
return;
}
else {
SYS_ARCH_INC(conn->recv_avail, p->tot_len);
API_EVENT(conn, NETCONN_EVT_RCVPLUS, p->tot_len);
signal recv_count_sema;
}

The semaphore would have to be used by all instances who access the queue and variables, so this approach is more complex, but safer.
These are used from netconn_recv, lwip_selscann and recv_raw.  Any other places?

Have I missed something, or is the count kept in parallell in two places:  recv_avail and in the form of the API_EVENT?

Steinar Lieng Fredriksen <steinarlf>
Fri 30 May 2008 01:53:26 PM UTC, comment #2: 

I looked at it, but not closely enough to be sure about whether it's safe to change the order of the trypost, recv_avail increment, and event callback. Certainly I believe the counters are not the crucial bit in the code affected - the API_EVENT call is.

I'm not sure the proposed fix would be reliable. Consider the following example scenario with my belief (not proved empirically) of what can happen:

1) TCPIP thread does successful trypost to recvmbox
2) Application thread pre-empts, calls lwip_read, which calls lwip_recvfrom which calls netconn_recv
3) netconn_recv fetches data from recvmbox
4) netconn_recv decrements conn->recv_avail, which being u16_t wraps round to 65535
4) netconn_recv calls API_EVENT(conn, NETCONN_EVT_RCVMINUS, len);
5) That calls event_callback(), and sock->rcvevent is a u16_t and now wraps round to 65535.
6) app calls lwip_select which calls lwip_selscan which tests rcvevent which, being 65535, it considers ready for reading even though it isn't.

I've considered doing a NETCONN_EVT_RCVPLUS event and then follow it immediately with an NETCONN_EVT_RCVMINUS on error, but select will still have woken the thread. And if the thread is higher priority than the tcpip thread it may run and be attempting a read before we were able to do the RCVMINUS.

Perhaps it would be sufficient to change rcvevent (and recv_avail for good measure) to signed quantities and check where they are tested to treat negative like 0 when deciding if there's enough space. Then the suggested fix may work.

BTW, this also affects recv_raw.

Incidentally, I'm not sure about the LWIP_DEBUGF addition. If that's to be there are at all then it should be API_MSG_DEBUG.

Jonathan Larmour <jifl>
Project Member
Fri 30 May 2008 12:01:41 PM UTC, comment #1: 

Does anyone have any thoughts on the suggested fix here.  It sounds OK to me but I've not looked in detail at what this counter is used for.

Kieran Mansley <kieranm>
Project Member
Tue 13 May 2008 10:51:07 AM UTC, original submission:  

If the trypost fails the counters are not decreased, leading to potential errors later. I had lwip_recvfrom block even though it was set to nonblocking mode.

I don't know if delaying the actual increase of the counters affect anything. This is done in the context of the tcpip thread, but might affect the raw api?

api_msg.c, line 169-175:
  SYS_ARCH_INC(conn->recv_avail, p->tot_len);
  /* Register event with callback */
  API_EVENT(conn, NETCONN_EVT_RCVPLUS, p->tot_len);
  if (sys_mbox_trypost(conn->recvmbox, buf) != ERR_OK) {
    netbuf_delete(buf);
    return;
  }

Proposed fix:
  if (sys_mbox_trypost(conn->recvmbox, buf) != ERR_OK) { 
    LWIP_DEBUGF(NETBUFERR,("recv_udp !!! TRYPOST FAILED\n"));
    netbuf_delete(buf);
    return;
  }
  else {
    SYS_ARCH_INC(conn->recv_avail, p->tot_len);
    /* Register event with callback */
    API_EVENT(conn, NETCONN_EVT_RCVPLUS, p->tot_len); 
  }

Steinar Lieng Fredriksen <steinarlf>

 

Attached Files
file #18330:  bug23240 added by kieranm (5KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by likafann (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by wdumon (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by steinarlf (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

     

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-07-27 kieranm StatusNone => Fixed
        Open/ClosedOpen => Closed
    2009-06-26 kieranm Attached File- => Added bug23240, #18330
    2008-05-30 kieranm Planned Release => 1.3.1

    Back to the top


    Powered by Savane 3.5