From 361d85ae355d047ca2ff3e4f786d22353e70357c Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Sat, 27 Jul 2019 10:26:52 -0600 Subject: [PATCH] net/tcp and udp: Fix errors in the new implementation of SO_LINGER. The tcp_drain() and udp_drain() functions were casting the working argument to the wrong type, resulting in hangs and abnormal behavior. There is a complexity in the tcp drain logic when the remote peer closes the socket before all Tx data has been flushed. Sometimes we are not notified of this case and wait the entire timeout unnecessarily. There is a workaround in place in tcp_txdrain(), but this really should be revisited. --- net/tcp/tcp.h | 3 +- net/tcp/tcp_notifier.c | 4 +-- net/tcp/tcp_txdrain.c | 80 ++++++++++++++++++++++++++++++++++-------- net/udp/udp_txdrain.c | 22 ++++++++++-- 4 files changed, 88 insertions(+), 21 deletions(-) diff --git a/net/tcp/tcp.h b/net/tcp/tcp.h index 4a244006d17..2350ac40202 100644 --- a/net/tcp/tcp.h +++ b/net/tcp/tcp.h @@ -1631,8 +1631,7 @@ int tcp_writebuffer_notifier_setup(worker_t worker, * > 0 - The signal notification is in place. The returned value is a * key that may be used later in a call to * tcp_notifier_teardown(). - * == 0 - There is already buffered read-ahead data. No signal - * notification will be provided. + * == 0 - No connection has been established. * < 0 - An unexpected error occurred and no signal will be sent. The * returned value is a negated errno value that indicates the * nature of the failure. diff --git a/net/tcp/tcp_notifier.c b/net/tcp/tcp_notifier.c index c9a9861a2ea..d106b5adc0a 100644 --- a/net/tcp/tcp_notifier.c +++ b/net/tcp/tcp_notifier.c @@ -41,6 +41,7 @@ #include #include +#include #include #include @@ -188,8 +189,7 @@ int tcp_writebuffer_notifier_setup(worker_t worker, * Returned Value: * > 0 - The notification is in place. The returned value is a key that * may be used later in a call to tcp_notifier_teardown(). - * == 0 - There is already buffered read-ahead data. No notification - * will be provided. + * == 0 - No connection has been established. * < 0 - An unexpected error occurred and notification will occur. The * returned value is a negated errno value that indicates the * nature of the failure. diff --git a/net/tcp/tcp_txdrain.c b/net/tcp/tcp_txdrain.c index 91c63ce617a..96d9171e404 100644 --- a/net/tcp/tcp_txdrain.c +++ b/net/tcp/tcp_txdrain.c @@ -46,6 +46,7 @@ #include #include +#include #include #include @@ -73,8 +74,24 @@ static void txdrain_worker(FAR void *arg) { - FAR sem_t *waitsem = (FAR sem_t *)arg; - DEBUGASSERT(waitsem != NULL); + /* The entire notifier entry is passed to us. That is because we are + * responsible for disposing of the entry via kmm_free() when we are + * finished with it. + */ + + FAR struct work_notifier_entry_s *notifier = + (FAR struct work_notifier_entry_s *)arg; + FAR sem_t *waitsem; + + DEBUGASSERT(notifier != NULL && notifier->info.arg != NULL); + waitsem = (FAR sem_t *)notifier->info.arg; + + /* Free the notifier entry */ + + kmm_free(notifier); + + /* Then just post the semaphore, waking up tcp_txdrain() */ + sem_post(waitsem); } @@ -106,7 +123,7 @@ int tcp_txdrain(FAR struct socket *psock, int ret; DEBUGASSERT(psock != NULL && psock->s_crefs > 0 && psock->s_conn != NULL); - DEBUGASSERT(psock->s_type == SOCK_DGRAM); + DEBUGASSERT(psock->s_type == SOCK_STREAM); conn = (FAR struct tcp_conn_s *)psock->s_conn; @@ -117,25 +134,60 @@ int tcp_txdrain(FAR struct socket *psock, /* The following needs to be done with the network stable */ net_lock(); + + /* Get a notification when the write buffers are drained */ + ret = tcp_writebuffer_notifier_setup(txdrain_worker, conn, &waitsem); + + /* The special return value of 0 means that there is no Tx data to be + * drained. Otherwise it is a special 'key' that can be used to teardown + * the notification later + */ + if (ret > 0) { - int key = ret; + /* Save the drain key */ - /* There is pending write data.. wait for it to drain. */ + int drain_key = ret; - do - { - ret = net_timedwait(&waitsem, abstime); + /* Also get a notification if we lose the connection. + * REVISIT: This really should not be necessary. tcp_send() should + * detect the disconnection event and signal the txdrain but I do not + * see that TCP disconnection event. This assures that the txdrain + * operation will not wait for the full timeout in that case. + */ + + ret = tcp_disconnect_notifier_setup(txdrain_worker, conn, &waitsem); + + /* Zero is a special value that means that no connection has been + * established. Otherwise it is a special 'key' that can be used + * to teardown the notification later + */ + + if (ret > 0) + { + /* Save the disconnect key */ + + int disconn_key = ret; + + /* There is pending write data and the socket is connected.. + * wait for it to drain or be be disconnected. + */ + + do + { + ret = net_timedwait(&waitsem, abstime); + } + while (ret == EINTR); + + /* Tear down the disconnect notifier */ + + tcp_notifier_teardown(disconn_key); } - while (ret == EINTR); - /* Tear down the notifier (in case we timed out or were canceled) */ + /* Tear down the disconnect notifier */ - if (ret < 0) - { - tcp_notifier_teardown(key); - } + tcp_notifier_teardown(drain_key); } net_unlock(); diff --git a/net/udp/udp_txdrain.c b/net/udp/udp_txdrain.c index d20df793a4e..ef20506e17d 100644 --- a/net/udp/udp_txdrain.c +++ b/net/udp/udp_txdrain.c @@ -64,7 +64,7 @@ * Called with the write buffers have all been sent. * * Input Parameters: - * arg - The semaphore that will wake up udp_txdrain + * arg - The notifier entry. * * Returned Value: * None. @@ -73,8 +73,24 @@ static void txdrain_worker(FAR void *arg) { - FAR sem_t *waitsem = (FAR sem_t *)arg; - DEBUGASSERT(waitsem != NULL); + /* The entire notifier entry is passed to us. That is because we are + * responsible for disposing of the entry via kmm_free() when we are + * finished with it. + */ + + FAR struct work_notifier_entry_s *notifier = + (FAR struct work_notifier_entry_s *)arg; + FAR sem_t *waitsem; + + DEBUGASSERT(notifier != NULL && notifier->info.arg != NULL); + waitsem = (FAR sem_t *)notifier->info.arg; + + /* Free the notifier entry */ + + kmm_free(notifier); + + /* Then just post the semaphore, waking up tcp_txdrain() */ + sem_post(waitsem); }