From bb86a6f81ec20b4e3238feeeac73839b65365a9b Mon Sep 17 00:00:00 2001 From: Tomer Cabouly Date: Mon, 31 Mar 2025 08:06:48 +0000 Subject: [PATCH] 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 --- src/core/dev/ring_slave.cpp | 4 ++-- src/core/sock/sockinfo_tcp.cpp | 5 +++-- src/utils/lock_wrapper.h | 8 ++++++++ tests/gtest/tcp/tcp_socket.cc | 27 ++++++++++++++++++++++++++- 4 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/core/dev/ring_slave.cpp b/src/core/dev/ring_slave.cpp index 7148b1855..f6416d260 100644 --- a/src/core/dev/ring_slave.cpp +++ b/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(multilock::create_new_lock(MULTILOCK_RECURSIVE, name)) - : static_cast(&t_lock_dummy_ring)); + : static_cast(&g_lock_dummy_ring.lock)); } ring_slave::ring_slave(int if_index, ring *parent, ring_type_t type, bool use_locks) diff --git a/src/core/sock/sockinfo_tcp.cpp b/src/core/sock/sockinfo_tcp.cpp index 5ece6dfbb..9e63f1628 100644 --- a/src/core/sock/sockinfo_tcp.cpp +++ b/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(multilock::create_new_lock(MULTILOCK_RECURSIVE, "tcp_con")) - : static_cast(&t_lock_dummy_socket)); + : static_cast(&g_lock_dummy_socket.lock)); } inline void sockinfo_tcp::lwip_pbuf_init_custom(mem_buf_desc_t *p_desc) diff --git a/src/utils/lock_wrapper.h b/src/utils/lock_wrapper.h index 65e3467c5..3aaf63bd2 100644 --- a/src/utils/lock_wrapper.h +++ b/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(); diff --git a/tests/gtest/tcp/tcp_socket.cc b/tests/gtest/tcp/tcp_socket.cc index c15cc1e87..dd4cb52ee 100644 --- a/tests/gtest/tcp/tcp_socket.cc +++ b/tests/gtest/tcp/tcp_socket.cc @@ -9,7 +9,7 @@ #include "common/sys.h" #include "common/base.h" #include "tcp_base.h" - +#include 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); +}