issue: 4347777 Replace thread-local dummy lock

The thread-local dummy locker in ring_slave could cause use-after-free
issues during XLIO shutdown when one thread attempts to access
a socket's locker that was created by a terminated thread.
This occurs because the thread-local object is freed
when its creator thread terminates.

Replace the thread-local dummy locker with a global one to prevent this
issue. To maintain data path performance, optimize the dummy lock for
a different cache-line to prevent false sharing by aligning the lock on
a 64-byte boundary.

Signed-off-by: Tomer Cabouly <[email protected]>

Author: tomerdbz

src/core/dev/ring_slave.cpp

@@ -21,13 +21,13 @@
 // AF_INET address 0.0.0.0:0, used for 3T flow spec keys.
 static const sock_addr s_sock_addrany;
 
-static thread_local lock_dummy t_lock_dummy_ring;
+static padded_lock_dummy g_lock_dummy_ring;
 
 static lock_base *get_new_lock(const char *name, bool real_lock)
 {
     return (real_lock
                 ? static_cast<lock_base *>(multilock::create_new_lock(MULTILOCK_RECURSIVE, name))
-                : static_cast<lock_base *>(&t_lock_dummy_ring));
+                : static_cast<lock_base *>(&g_lock_dummy_ring.lock));
 }
 
 ring_slave::ring_slave(int if_index, ring *parent, ring_type_t type, bool use_locks)

src/core/sock/sockinfo_tcp.cpp

@@ -57,7 +57,8 @@ extern global_stats_t g_global_stat_static;
 tcp_timers_collection *g_tcp_timers_collection = nullptr;
 thread_local thread_local_tcp_timers g_thread_local_tcp_timers;
 bind_no_port *g_bind_no_port = nullptr;
-static thread_local lock_dummy t_lock_dummy_socket;
+
+static padded_lock_dummy g_lock_dummy_socket;
 
 /*
  * The following socket options are inherited by a connected TCP socket from the listening socket:
@@ -137,7 +138,7 @@ static lock_base *get_new_tcp_lock()
     return (
         safe_mce_sys().tcp_ctl_thread != option_tcp_ctl_thread::CTL_THREAD_DELEGATE_TCP_TIMERS
             ? static_cast<lock_base *>(multilock::create_new_lock(MULTILOCK_RECURSIVE, "tcp_con"))
-            : static_cast<lock_base *>(&t_lock_dummy_socket));
+            : static_cast<lock_base *>(&g_lock_dummy_socket.lock));
 }
 
 inline void sockinfo_tcp::lwip_pbuf_init_custom(mem_buf_desc_t *p_desc)

src/utils/lock_wrapper.h

@@ -473,6 +473,14 @@ class lock_dummy : public lock_base {
     int is_locked_by_me() override { return 1; }
 };
 
+// Users of lock_dummy may wish to alignas(64) to place this lock in in a different cache-line and
+// prevent false sharing
+struct alignas(64) padded_lock_dummy {
+    lock_dummy lock;
+    // Padding to fill a full cache line
+    char padding[64 - sizeof(lock_dummy)];
+};
+
 static inline void lock_deleter_func(lock_base *lock)
 {
     lock->delete_obj();

tests/gtest/tcp/tcp_socket.cc

@@ -9,7 +9,7 @@
 #include "common/sys.h"
 #include "common/base.h"
 #include "tcp_base.h"
-
+#include <thread>
 class tcp_socket : public tcp_base {};
 
 /**
@@ -125,3 +125,28 @@ TEST_F(tcp_socket, ti_2_ipv6only_listen_all)
     test_lambda(true);
     test_lambda(false);
 }
+
+/**
+ * @test tcp_socket.ti_3_socket_closed_different_thread_works
+ * @brief
+ *    Test that a socket can be closed after its creator thread terminates
+ * @details
+ *    Creates a socket in a separate thread, then closes it from the main thread
+ *    after the creator thread has terminated. This verifies that socket cleanup
+ *    works correctly across thread boundaries.
+ */
+TEST_F(tcp_socket, ti_3_socket_closed_different_thread_works)
+{
+    int fd = -1;
+
+    std::thread t([&fd]() {
+        fd = socket(m_family, SOCK_STREAM, IPPROTO_IP);
+        EXPECT_LE(0, fd);
+        EXPECT_EQ(errno, EOK);
+    });
+
+    t.join();
+
+    EXPECT_LE(0, fd);
+    close(fd);
+}