This is an automated email from the ASF dual-hosted git repository. xiaoxiang pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-nuttx.git
commit 98ec46d726b4b58e58f8fc149a38628d7dde193e Author: YAMAMOTO Takashi <[email protected]> AuthorDate: Tue Jul 20 09:10:43 2021 +0900 tcp_send_buffered.c: fix iob allocation deadlock Ensure to put the wrb back onto the write_q when blocking on iob allocation. Otherwise, it can deadlock with other threads doing the same thing. --- net/tcp/tcp_send_buffered.c | 165 ++++++++++++++++++++++++-------------------- 1 file changed, 90 insertions(+), 75 deletions(-) diff --git a/net/tcp/tcp_send_buffered.c b/net/tcp/tcp_send_buffered.c index 2743dca..239107e 100644 --- a/net/tcp/tcp_send_buffered.c +++ b/net/tcp/tcp_send_buffered.c @@ -1129,69 +1129,73 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf, } #endif /* CONFIG_NET_SEND_BUFSIZE */ - /* Allocate a write buffer. Careful, the network will be momentarily - * unlocked here. - */ + while (true) + { + struct iob_s *iob; - /* Try to coalesce into the last wrb. - * - * But only when it might yield larger segments. - * (REVISIT: It might make sense to lift this condition. - * IOB boundaries and segment boundaries usually do not match. - * It makes sense to save the number of IOBs.) - * - * Also, for simplicity, do it only when we haven't sent anything - * from the the wrb yet. - */ + /* Allocate a write buffer. Careful, the network will be + * momentarily unlocked here. + */ - max_wrb_size = tcp_max_wrb_size(conn); - wrb = (FAR struct tcp_wrbuffer_s *)sq_tail(&conn->write_q); - if (wrb != NULL && TCP_WBSENT(wrb) == 0 && TCP_WBNRTX(wrb) == 0 && - TCP_WBPKTLEN(wrb) < max_wrb_size && - (TCP_WBPKTLEN(wrb) % conn->mss) != 0) - { - wrb = (FAR struct tcp_wrbuffer_s *)sq_remlast(&conn->write_q); - ninfo("coalesce %zu bytes to wrb %p (%" PRIu16 ")\n", len, wrb, - TCP_WBPKTLEN(wrb)); - DEBUGASSERT(TCP_WBPKTLEN(wrb) > 0); - } - else if (nonblock) - { - wrb = tcp_wrbuffer_tryalloc(); - ninfo("new wrb %p (non blocking)\n", wrb); - } - else - { - wrb = tcp_wrbuffer_alloc(); - ninfo("new wrb %p\n", wrb); - } + /* Try to coalesce into the last wrb. + * + * But only when it might yield larger segments. + * (REVISIT: It might make sense to lift this condition. + * IOB boundaries and segment boundaries usually do not match. + * It makes sense to save the number of IOBs.) + * + * Also, for simplicity, do it only when we haven't sent anything + * from the the wrb yet. + */ - if (wrb == NULL) - { - /* A buffer allocation error occurred */ + max_wrb_size = tcp_max_wrb_size(conn); + wrb = (FAR struct tcp_wrbuffer_s *)sq_tail(&conn->write_q); + if (wrb != NULL && TCP_WBSENT(wrb) == 0 && TCP_WBNRTX(wrb) == 0 && + TCP_WBPKTLEN(wrb) < max_wrb_size && + (TCP_WBPKTLEN(wrb) % conn->mss) != 0) + { + wrb = (FAR struct tcp_wrbuffer_s *)sq_remlast(&conn->write_q); + ninfo("coalesce %zu bytes to wrb %p (%" PRIu16 ")\n", len, wrb, + TCP_WBPKTLEN(wrb)); + DEBUGASSERT(TCP_WBPKTLEN(wrb) > 0); + } + else if (nonblock) + { + wrb = tcp_wrbuffer_tryalloc(); + ninfo("new wrb %p (non blocking)\n", wrb); + DEBUGASSERT(TCP_WBPKTLEN(wrb) == 0); + } + else + { + wrb = tcp_wrbuffer_alloc(); + ninfo("new wrb %p\n", wrb); + DEBUGASSERT(TCP_WBPKTLEN(wrb) == 0); + } - nerr("ERROR: Failed to allocate write buffer\n"); - ret = nonblock ? -EAGAIN : -ENOMEM; - goto errout_with_lock; - } + if (wrb == NULL) + { + /* A buffer allocation error occurred */ - /* Initialize the write buffer */ + nerr("ERROR: Failed to allocate write buffer\n"); + ret = nonblock ? -EAGAIN : -ENOMEM; + goto errout_with_lock; + } - TCP_WBSEQNO(wrb) = (unsigned)-1; - TCP_WBNRTX(wrb) = 0; + /* Initialize the write buffer */ - off = TCP_WBPKTLEN(wrb); - if (off + chunk_len > max_wrb_size) - { - chunk_len = max_wrb_size - off; - } + TCP_WBSEQNO(wrb) = (unsigned)-1; + TCP_WBNRTX(wrb) = 0; - /* Copy the user data into the write buffer. We cannot wait for - * buffer space if the socket was opened non-blocking. - */ + off = TCP_WBPKTLEN(wrb); + if (off + chunk_len > max_wrb_size) + { + chunk_len = max_wrb_size - off; + } + + /* Copy the user data into the write buffer. We cannot wait for + * buffer space. + */ - if (nonblock) - { /* The return value from TCP_WBTRYCOPYIN is either OK or * -ENOMEM if less than the entire data chunk could be allocated. * If -ENOMEM is returned, check if at least a part of the data @@ -1220,34 +1224,48 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf, } else { - nerr("ERROR: Failed to add data to the I/O chain\n"); - ret = -EWOULDBLOCK; - goto errout_with_wrb; + chunk_result = 0; } } else { DEBUGASSERT(chunk_result == chunk_len); } - } - else - { - unsigned int count; - int blresult; - /* iob_copyin might wait for buffers to be freed, but if network is - * locked this might never happen, since network driver is also - * locked, therefore we need to break the lock - */ + if (chunk_result > 0) + { + break; + } + + /* release wrb */ - blresult = net_breaklock(&count); - ninfo("starting copyin to wrb %p\n", wrb); - chunk_result = TCP_WBCOPYIN(wrb, cp, chunk_len, off); - ninfo("finished copyin to wrb %p\n", wrb); - if (blresult >= 0) + if (TCP_WBPKTLEN(wrb) > 0) { - net_restorelock(count); + DEBUGASSERT(TCP_WBSENT(wrb) == 0); + DEBUGASSERT(TCP_WBPKTLEN(wrb) > 0); + sq_addlast(&wrb->wb_node, &conn->write_q); } + else + { + tcp_wrbuffer_release(wrb); + } + + if (nonblock) + { + nerr("ERROR: Failed to add data to the I/O chain\n"); + ret = -EAGAIN; + goto errout_with_lock; + } + + /* Wait for at least one IOB getting available. + * + * Note: net_ioballoc releases the network lock when blocking. + * It allows our write_q being drained in the meantime. Otherwise, + * we risk a deadlock with other threads competing on IOBs. + */ + + iob = net_ioballoc(true, IOBUSER_NET_TCP_WRITEBUFFER); + iob_free_chain(iob, IOBUSER_NET_TCP_WRITEBUFFER); } /* Dump I/O buffer chain */ @@ -1311,9 +1329,6 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf, return result; -errout_with_wrb: - tcp_wrbuffer_release(wrb); - errout_with_lock: net_unlock();
