From e46688c1ee08e4bdc76184d8a6c640a22ab4f42c Mon Sep 17 00:00:00 2001
From: chao an <anchao@xiaomi.com>
Date: Tue, 30 Aug 2022 11:04:31 +0800
Subject: [PATCH] net/tcp: use independent work to free the conn instance

I noticed that the conn instance will leak during stress test,
The close work queued from tcp_close_eventhandler() will be canceled
by tcp_timer() immediately:

Breakpoint 1, tcp_close_eventhandler (dev=0x565cd338 <up_irq_restore+108>, pvpriv=0x5655e6ff <getpid+12>, flags=0) at tcp/tcp_close.c:71
(gdb) bt
| #0  tcp_close_eventhandler (dev=0x565cd338 <up_irq_restore+108>, pvpriv=0x5655e6ff <getpid+12>, flags=0) at tcp/tcp_close.c:71
| #1  0x5658bf1e in devif_conn_event (dev=0x5660bd80 <g_sim_dev>, flags=512, list=0x5660d558 <g_cbprealloc+312>) at devif/devif_callback.c:508
| #2  0x5658a219 in tcp_callback (dev=0x5660bd80 <g_sim_dev>, conn=0x5660c4a0 <g_tcp_connections>, flags=512) at tcp/tcp_callback.c:167
| #3  0x56589253 in tcp_timer (dev=0x5660bd80 <g_sim_dev>, conn=0x5660c4a0 <g_tcp_connections>) at tcp/tcp_timer.c:378
| #4  0x5658dd47 in tcp_poll (dev=0x5660bd80 <g_sim_dev>, conn=0x5660c4a0 <g_tcp_connections>) at tcp/tcp_devpoll.c:95
| #5  0x5658b95f in devif_poll_tcp_connections (dev=0x5660bd80 <g_sim_dev>, callback=0x565770f2 <netdriver_txpoll>) at devif/devif_poll.c:601
| #6  0x5658b9ea in devif_poll (dev=0x5660bd80 <g_sim_dev>, callback=0x565770f2 <netdriver_txpoll>) at devif/devif_poll.c:722
| #7  0x56577230 in netdriver_txavail_work (arg=0x5660bd80 <g_sim_dev>) at sim/up_netdriver.c:308
| #8  0x5655999e in work_thread (argc=2, argv=0xf3db5dd0) at wqueue/kwork_thread.c:178
| #9  0x5655983f in nxtask_start () at task/task_start.c:129

(gdb) c
Continuing.
Breakpoint 2, tcp_update_timer (conn=0x5660c4a0 <g_tcp_connections>) at tcp/tcp_timer.c:178
(gdb) bt
| #0  tcp_update_timer (conn=0x5660c4a0 <g_tcp_connections>) at tcp/tcp_timer.c:178
| #1  0x5658952a in tcp_timer (dev=0x5660bd80 <g_sim_dev>, conn=0x5660c4a0 <g_tcp_connections>) at tcp/tcp_timer.c:708
| #2  0x5658dd47 in tcp_poll (dev=0x5660bd80 <g_sim_dev>, conn=0x5660c4a0 <g_tcp_connections>) at tcp/tcp_devpoll.c:95
| #3  0x5658b95f in devif_poll_tcp_connections (dev=0x5660bd80 <g_sim_dev>, callback=0x565770f2 <netdriver_txpoll>) at devif/devif_poll.c:601
| #4  0x5658b9ea in devif_poll (dev=0x5660bd80 <g_sim_dev>, callback=0x565770f2 <netdriver_txpoll>) at devif/devif_poll.c:722
| #5  0x56577230 in netdriver_txavail_work (arg=0x5660bd80 <g_sim_dev>) at sim/up_netdriver.c:308
| #6  0x5655999e in work_thread (argc=2, argv=0xf3db5dd0) at wqueue/kwork_thread.c:178
| #7  0x5655983f in nxtask_start () at task/task_start.c:129

Since a separate work will add 24 bytes to each conn instance,
but in order to support the feature of asynchronous close(),
I can not find a better way than adding a separate work,
for resource constraints, I recommend the developers to enable
CONFIG_NET_ALLOC_CONNS, which will reduce the ram usage.

Signed-off-by: chao an <anchao@xiaomi.com>
---
 net/tcp/tcp.h       |  1 +
 net/tcp/tcp_close.c | 17 ++++++++++++-----
 net/tcp/tcp_conn.c  |  6 ++++++
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/net/tcp/tcp.h b/net/tcp/tcp.h
index 8c3068768f3e5..9647b92bca9f5 100644
--- a/net/tcp/tcp.h
+++ b/net/tcp/tcp.h
@@ -304,6 +304,7 @@ struct tcp_conn_s
   /* Reference to TCP close callback instance */
 
   FAR struct devif_callback_s *clscb;
+  struct work_s                clswork;
 
 #if defined(CONFIG_NET_TCP_WRITE_BUFFERS)
   /* Callback instance for TCP send() */
diff --git a/net/tcp/tcp_close.c b/net/tcp/tcp_close.c
index 4a98c9833e643..86fc22a6e94bd 100644
--- a/net/tcp/tcp_close.c
+++ b/net/tcp/tcp_close.c
@@ -53,10 +53,13 @@ static void tcp_close_work(FAR void *param)
 
   net_lock();
 
-  /* Stop the network monitor for all sockets */
+  if (conn && conn->crefs == 0)
+    {
+      /* Stop the network monitor for all sockets */
 
-  tcp_stop_monitor(conn, TCP_CLOSE);
-  tcp_free(conn);
+      tcp_stop_monitor(conn, TCP_CLOSE);
+      tcp_free(conn);
+    }
 
   net_unlock();
 }
@@ -175,11 +178,15 @@ static uint16_t tcp_close_eventhandler(FAR struct net_driver_s *dev,
   return flags;
 
 end_wait:
-  tcp_callback_free(conn, conn->clscb);
+  if (conn->clscb != NULL)
+    {
+      tcp_callback_free(conn, conn->clscb);
+      conn->clscb = NULL;
+    }
 
   /* Free network resources */
 
-  work_queue(LPWORK, &conn->work, tcp_close_work, conn, 0);
+  work_queue(LPWORK, &conn->clswork, tcp_close_work, conn, 0);
 
   return flags;
 }
diff --git a/net/tcp/tcp_conn.c b/net/tcp/tcp_conn.c
index 2532eb8ab1c38..4c3e2990b5127 100644
--- a/net/tcp/tcp_conn.c
+++ b/net/tcp/tcp_conn.c
@@ -769,6 +769,12 @@ void tcp_free(FAR struct tcp_conn_s *conn)
 
   DEBUGASSERT(conn->crefs == 0);
 
+  /* Cancel close work */
+
+  work_cancel(LPWORK, &conn->clswork);
+
+  /* Cancel tcp timer */
+
   tcp_stop_timer(conn);
 
   /* Free remaining callbacks, actually there should be only the send