Fri 25 Mar 2016 04:13:17 PM UTC, comment #22:
Fixed by implementing the flag and always calling tcp_backlog_accepted() from tcp_pcb_purge().
|
Fri 25 Mar 2016 09:41:59 AM UTC, comment #21:
> All that should be needed is a flag in the PCB
I already had implemented it like that (that's why I had to do an additional "fix" commit): tcp_flagst_t gets u16_t always. I just thought it wasn't necessary, but now I see it is.
|
Thu 24 Mar 2016 11:02:41 PM UTC, comment #20:
Ambroz,
I agree we could also store some state in the child PCB that it's part of the current listener backlog count. That would eliminate the need to maintain a linked list
|
Thu 24 Mar 2016 05:14:53 PM UTC, comment #19:
Hey,
I think that another linked list is unwarranted. All that should be needed is a flag in the PCB indicating if the PCB has an accepts_pending reference. Or use the "listener" pointer for this as I suggested (if it's not NULL, we have a reference), but rename it to something like backlog_ref_listener to avoid confusion.
|
Thu 24 Mar 2016 03:12:47 PM UTC, comment #18:
I also took a look at the changes and agree the inconsistent state situations seems to come about because the upper layers are currently required to perform a matching pair of tcp_backlog_delayed() and tcp_backlog_accepted() calls. If the child PCB is freed internally before this happens and is in state ESTABLISHED, we have a leak in the listener.
The bulk of the problem seems to stem from not knowing if the child PCB (once in state ESTABLISHED) being freed is actually on the backlog or not.
One way I could think to solve this would be to maintain a linked list of child PBCs in the listener PCB in-addition to the count. Inserts into the list would be O(1) by inserting at head, removals would be more expensive in we would have to traverse the list. Operations to check the backlog size could use the count in order to avoid a traversal
This would then allow TCP internal to free PCBs without the matching tcp_backlog_accepted() call from the upper layers
|
Thu 24 Mar 2016 07:33:29 AM UTC, comment #17:
The internal backlog should be OK I think.
The external 'delayed/accepted' mechanism might really get inconsistent. I'll have to think about a fix. Setting 'listener' to NULL in 'accepted' doesn't seem right, as it is already used internally. Plus the listener pointer might (in future?) be used for other stuff...
|
Wed 23 Mar 2016 09:47:40 PM UTC, comment #16:
Ah, forget about the part regarding tcp_abandon(), I missed that it calls TCP_PCB_REMOVE_ACTIVE-->tcp_pcb_remove-->tcp_pcb_purge.
|
Wed 23 Mar 2016 09:42:19 PM UTC, comment #15:
Hello Simon,
I looked at your change and I think there is an issue with PCB errors as indicated by the tcp_err callback. If there is an error after the application has used tcp_backlog_delayed() (and before tcp_backlog_accepted()):
- It is generally unsafe to call tcp_backlog_accepted() from within the error callback, because the PCB memory may have been already deallocated (this is the case in 2 out of 4 possible error sources, just search for TCP_EVENT_ERR calls).
- The netconn code does not use tcp_backlog_delayed() in its error callback (err_tcp). This leads to a reference leak if there is an error while PCB is in the accept queue.
I think there is also a pre-existing reference leak of accept_pending in tcp_abandon(). This is triggered from tcp_abort(), tcp_alloc()/tcp_kill_state() and in tcp_listen_input() if tcp_enqueue_flags fails.
Regarding solving these issues, I suggest to change tcp_backlog_accepted() to also set pcb->listener to NULL (enabling safe usage if called redundantly), and call this from relevant places (tcp_pcb_purge, tcp_abandon). Maybe also in tcp_close_shutdown in some cases but I'm not familiar with the closing logic.
|
Wed 23 Mar 2016 07:45:20 AM UTC, comment #14:
Fixed by changing accept and backlog: instead of having an accept callback in the cnnection pcb, there is now a pointer to the listening pcb. That way, accepts_pending can be decreased very fast. This is now done internally in the stack ('tcp_accepted()' got useless now).
An application can delay decreasing 'accepts_pending' by manually calling 'tcp_backlog_delayed()' and 'tcp_backlog_accepted()' (which is what netconns/sockets now do).
The only downside is that when a listener is closed, all tcp pcb lists have to be traversed to reset the 'listener' pointer, but that should happen so often anyway...
|
Sun 06 Mar 2016 05:18:06 PM UTC, comment #13:
I'd be glad to get a better working version of the "accepts_pending" implementation. The current way of letting the user application keeping track of the corresponding listen pcb is really bad.
I'd favour an implementation where the stack decreases "accepts_pending" before calling the "accept" callback and the application has to call API functions (on the connection pcb) to increase/decrease "accepts_pending" (or a 2nd counter) when connections are put on a queue.
One possible improvement would be to remove the "accept" callback from non-listening pcbs and giving them a pointer to their listen-pcb instead (which has the accept callback pointer). Closing a listen-pcb would require walking all pcb lists, but that could be worth it.
|
Thu 21 Jan 2016 05:59:07 PM UTC, comment #12:
Also netconn should be releasing the reference in the error paths in accept_function(). Assuming that the lwip core is not changed with respect to releasing the reference itself.
|
Thu 21 Jan 2016 05:56:51 PM UTC, comment #11:
Hey Joel,
Isn't the problem with not having the listener reference only in tcp_accept_null? That one can just do the same thing as tcp_pcb_purge does - find the listener. In my patch this is the function tcp_pcb_decrement_accepts_pending_of_listener(). So all that needs to be done is to fix tcp_accept_null:
static err_t tcp_accept_null(void arg, struct tcp_pcb pcb, err_t err)
{
LWIP_UNUSED_ARG(arg);
LWIP_UNUSED_ARG(pcb);
LWIP_UNUSED_ARG(err);
tcp_pcb_decrement_accepts_pending_of_listener(pcb);
tcp_abort(pcb);
return ERR_ABRT;
}
On the other hand I'm not so sure the netconn is handling the accept reference right. For one thing it doesn't seem to release it in the PCB's error callback (err_tcp). Also I find the tcp_accepted() in lwip_netconn_do_recv() suspicions.
Please be aware though that I don't have a real interest in netconn...
|
Thu 21 Jan 2016 04:58:43 PM UTC, comment #10:
Ambroz,
I think we both proposed the same solution, sorry if I wasn't clear :)
I agree that the accept callback could also take responsibility for decrementing accepts_pending in the failure case. As you mentioned, we would also need a reference to the listener PCB. I'm not sure how easily we can change the accept callback function signature to pass that because it would break existing raw applications, also we would need to do a look up in tcp_process()
The netconn layer actually does already have the reference in accept_function (api_msg.c) because it passed the listener netconn via tcp_arg(), but we would want to make the listener easily accessible for raw applications as well
|
Tue 19 Jan 2016 06:00:02 PM UTC, comment #9:
Also, I wouldn't complicate the situation by interpreting errors from the accept callback differently as long as the accepts_pending reference is concerned. The accept callback should assume ownership of a single reference regardless of what error code it returns.
|
Tue 19 Jan 2016 05:55:06 PM UTC, comment #8:
I think it doesn't matter if you manipulate the backlog or accepts_pending. If you want to have separate control over the backlog, incrementing/decrementing the accepts_pending instead of decrementing/incrementing the backlog makes sense.
But then the normal flow would be suboptimal:
(1) When SYN is received accepts_pending would be incremented.
(2) When SYN-ACK is received accepts_pending would be decremented just before the accept callback.
(3) The accept callback would increment it right back.
(4) When the socket is passed upward, accepts_pending would finally be decremented.
So maybe we should just not decrement accepts_pending in step (2), and effectively pass the reference to the accept callback. Just fix the tcp_accept_null to call tcp_accepted() or more specificaly an equivalent which find the lpcb, since we don't have it at hand.
Anything that keeps a queue of accepted connections between lwIP core and the application can deal with this reference properly. So:
- When finally passing the connections into accept, call tcp_accepted().
- If the PCB is killed by lwip before that, tcp_accepted() must be called from the PCB's error callback.
Note that this doesn't need any change in tcp_pcb_purge(). This one would keep being responsible just for decrementing accepts_pending of SYN_RCVD PCBs.
|
Tue 19 Jan 2016 05:30:26 PM UTC, comment #7:
Ambroz,
I understand the suggestion (thanks!) and have a couple of thoughts:
If the solution is to have the accept callbacks manage controlling the "connected, but not accepted" connections via manipulating the backlog value, then we could also implement a more clear and easier solution by having them manipulate accepts_pending if the callback fails to take the connection (returns !ERR_OK). Currently that's not a responsibility of the accept callback and where this issue arises from. Even the default tcp_accept_null() has the same issue
The backlog value stored within the TCB would no longer be the total count of both "in-progress connections" and "connected, but not accepted" queues. I have another patchset posted (https://savannah.nongnu.org/patch/?8764) that allows updating of the backlog value via another call to listen() which behaves as most BSD implementations do. This suggested solution doesn't play well with this feature because it could lead to some cases where the application increments the backlog while there are "connected, but not accepted" connections, this would then artificially inflate the backlog when those are accepted
The suggested patch also reduces performance by having every passive connection run through the listen PCB list where before this was avoided, at least for sockets/netconn, not sure about users of raw API.
I like the solution of having TCP core handle bookkeeping on accepts_pending when the accept_callback() refuses/fails to take the connection.
|
Mon 18 Jan 2016 09:45:38 PM UTC, comment #6:
Note you don't even need to change core lwIP code to do this, just cast the tcp_pcb* into tcp_pcb_listen* and have fun with the ->backlog.
Attached is a patch implementing my solution in comment #3.
(file #36087)
|
Mon 18 Jan 2016 09:37:42 PM UTC, comment #5:
Hey Joel,
I wasn't considering that. But the solution is fine for applications which use the raw API directly and do not delay accepting the connection.
Actually, I think there is a minimally invasive way to go from this design to one which limits SYN_RCVD plus the number of ESTABLISHED but not "fully accepted" connections. That is:
- In the accept callback, decrement lpcb->backlog as you queue up the connection for passing up to the application.
- Once you do finally pass the connection to the higher layer (the final "accept"), increment lpcb->backlog.
- Also take care to increment lpcb->backlog if you abandon the connection before passing it up.
By temporarily decrementing the lpcb->backlog, you effectively make lwIP consider your half-accepted connections in the backlog size check when SYN packets are received.
Actually, in my application I play with lpcb->backlog to limit the total number of PCBs used for a service, preventing unintended closing of connections for other services. To do this, I decrement lpcb->backlog in the accept callback, and only increment it back when the connection is closed.
|
Mon 18 Jan 2016 05:19:38 PM UTC, comment #4:
Ambroz,
Other network stacks maintain two queues of connections for the backlog: one for in-progress connections (SYN_RCVD) and a separate queue for completed connections (ESTABLISHED) that have not been accepted by the application. As the stack completes connections from the in-progress queue, the are moved to the completed queue and the application is notified it can call accept(). Once accept() is called, the completed queue is decremented. The backlog value is the count of both queues.
LwIP is in-affect implementing the same mechanism, but without actually maintaining queues/lists. The backlog value (accepts_pending) covers the count of both queues.
With your suggestion, we would be removing the queue of "completed but not accepted" connections from the backlog tracking
I think the problem is that since we aren't maintaining a list of PCBs that are ESTABLISHED but not accepted, we don't know in tcp_pcb_purge() when to decrement the accepts_pending value other than for PCBs in SYC_RCVD
|
Sun 17 Jan 2016 05:58:03 PM UTC, comment #3:
Hey,
I think there is a simple solution for this:
- In tcp_process(), decrement accepts_pending just as pcb->state is set to ESTABLISHED (from SYN_RCVD).
- Remove the decrement of accepts_pending in tcp_accepted().
My thinking is that accepts_pending should always be equal to the number of PCBs in SYN_RCVD state associated with the listener. So it makes sense to decrement it at the transition SYN_RCVD->ESTABLISHED. Also consider that tcp_pcb_purge() decrements it only when the PCB is in SYN_RCVD state.
|
Wed 16 Dec 2015 09:39:19 PM UTC, comment #2:
Thinking about this more, the tcp_accept_null() function also has the same problem (actually any accept callback that calls tcp_abort and returns ERR_ABRT). It calls tcp_abort(), but since the connection is in ESTABLISHED state, accepts_pending won't be decremented
The problem seems to stem from the fact that the connections are in the ESTABLISHED state and still in the backlog, but this knowledge isn't tracked at the TCP level and instead relies on upper layers (netconn's accept mbox) to associate the established connections with the listener. The upper layer is then responsible for calling tcp_accepted() when the pending connections are pulled off the backlog.
For this same reason we can't just adjust the tcp_pcb_purge() to check for ESTABLISHED PCBs in the backlog because TCP doesn't know which ESTABLISHED PCBs are in the backlog and which aren't. The SYN_RCVD check in tcp_pcb_purge() learns which connections are still in the backlog by matching the local port with a listener and using the SYN_RCVD state.
Maybe we need an actual backlog queue of PCBs in LwIP? I know other stacks have multiple queues depending on the state of the pending connection
|
Wed 16 Dec 2015 05:44:12 PM UTC, comment #1:
Attached is a patch for solution #2
|
Wed 16 Dec 2015 03:37:22 PM UTC, original submission:
I'm running LwIP master branch and in my product (NO_SYS = 0, sockets/netconn enabled), I've encountered the following behavior:
With a TCP listener, if a connection in SYN_RCVD receives its ACK, but calling TCP_EVENT_ACCEPT() returns an error (ERR_MEM in my case) then the new connection is aborted, but accepts_pending is not decreased on the parent listener PCB, causing a leak in accepts_pending
In the call tree of tcp_abort(), tcp_pcb_purge() does get called, which contains logic to decrease accepts_pending on the listener PCB, but only if the PCB is in SYN_RCVD and in our case, we advance the state to ESTABLISHED right before calling TCP_EVENT_ACCEPT
The referenced code is in tcp_in.c, tcp_process()
I had a couple of ideas:
- Don't advance the state to ESTABLISHED until after calling TCP_EVENT_ACCEPT
- Revert the state to SYN_RVCD before calling tcp_abort() in the error case
- Manually perform the booking keeping on the listener PCB in the error case of SYN_RCVD
I'm leaning towards #2 as that doesn't change anything when TCP_EVENT_ACCEPT() is called and doesn't introduce duplicated code
|