From db4aa24dbb5d7f090148d563b3cc740cbf65059a Mon Sep 17 00:00:00 2001 From: Evgeny Budilovsky Date: Tue, 2 Apr 2024 14:07:04 +0300 Subject: [PATCH 1/4] NEBDUTY-965: promise object passed as pointer to different thread released too early promise object is passed as pointer to different thread which will invoke SetValue on promise. The main thread which is waiting in `promise.GetFuture().Wait()` will get control and delete the promise variable keeping dangling pointer in the other thread. Since the calling thread won't access the dangling pointer nothing bad should happen but this triggers failure of unit tests compiled with tsan --- cloud/blockstore/vhost-server/server.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/cloud/blockstore/vhost-server/server.cpp b/cloud/blockstore/vhost-server/server.cpp index 100eec40dc6..f1aebfab915 100644 --- a/cloud/blockstore/vhost-server/server.cpp +++ b/cloud/blockstore/vhost-server/server.cpp @@ -180,14 +180,12 @@ void TServer::Stop() { STORAGE_INFO("Stopping the server"); - { - auto promise = NewPromise(); - vhd_unregister_blockdev(Handler, [] (void* opaque) { - static_cast*>(opaque)->SetValue(); - }, &promise); + auto promise = NewPromise(); + vhd_unregister_blockdev(Handler, [] (void* opaque) { + static_cast*>(opaque)->SetValue(); + }, &promise); - promise.GetFuture().Wait(); - } + promise.GetFuture().Wait(); // 2. Stop request queues. For each do: // 2.1 Stop a request queue From 76916a65db57ad9ea44a831e94095a31654ef61a Mon Sep 17 00:00:00 2001 From: Evgeny Budilovsky Date: Tue, 2 Apr 2024 15:35:39 +0300 Subject: [PATCH 2/4] NEBDUTY-965: Remove unsafe debug code in vhost-server aio backend This code is unsafe because it attempts to print the contents of a batch after it has been submitted. This can lead to use-after-free errors if the IOs in the batch are freed before the print function is called. Additionally, this code caused `make --sanitize=address -tt cloud/blockstore/vhost-server/ut` to fail due to use-after-free errors. --- cloud/blockstore/vhost-server/backend_aio.cpp | 54 ------------------- 1 file changed, 54 deletions(-) diff --git a/cloud/blockstore/vhost-server/backend_aio.cpp b/cloud/blockstore/vhost-server/backend_aio.cpp index 07ff6f0317f..15ba333561d 100644 --- a/cloud/blockstore/vhost-server/backend_aio.cpp +++ b/cloud/blockstore/vhost-server/backend_aio.cpp @@ -18,55 +18,6 @@ namespace { //////////////////////////////////////////////////////////////////////////////// -#ifndef NDEBUG - -TString ToString(std::span batch) -{ - TStringStream ss; - - const char* op[]{ - "pread", - "pwrite", - "", - "", - "", - "", - "", - "preadv", - "pwritev", - }; - - ss << "[ "; - for (iocb* cb: batch) { - ss << "{ " << op[cb->aio_lio_opcode] << ":" << cb->aio_fildes << " "; - switch (cb->aio_lio_opcode) { - case IO_CMD_PREAD: - case IO_CMD_PWRITE: - ss << cb->u.c.buf << " " << cb->u.c.nbytes << ":" - << cb->u.c.offset; - break; - case IO_CMD_PREADV: - case IO_CMD_PWRITEV: { - iovec* iov = static_cast(cb->u.c.buf); - for (unsigned i = 0; i != cb->u.c.nbytes; ++i) { - ss << "(" << iov[i].iov_base << " " << iov[i].iov_len - << ") "; - } - break; - } - } - ss << "} "; - } - - ss << "]"; - - return ss.Str(); -} - -#endif // NDEBUG - -//////////////////////////////////////////////////////////////////////////////// - void CompleteRequest( TAioRequest* req, vhd_bdev_io_result status, @@ -345,11 +296,6 @@ void TAioBackend::ProcessQueue( queueStats.Submitted += ret; -#ifndef NDEBUG - STORAGE_DEBUG( - "submitted " << ret << ": " - << ToString(std::span(batch.data(), ret))); -#endif // remove submitted items from the batch batch.erase(batch.begin(), batch.begin() + ret); } From 764cd549fa0c1498d162c8257af7d174742932ed Mon Sep 17 00:00:00 2001 From: Evgeny Budilovsky Date: Tue, 2 Apr 2024 17:18:38 +0300 Subject: [PATCH 3/4] NEBDUTY-965: help tsan to deduce happens-before relation properly in vhost-server vhost-server unit tests with tsan fail with error similar to ``` WARNING: ThreadSanitizer: data race (pid=2340732) Read of size 8 at 0x7b1c000070b0 by thread T2: #0 NCloud::NBlockStore::NVHostServer::(anonymous namespace)::TAioBackend::CompletionThreadFunc() /cloud/blockstore/vhost-server/backend_aio.cpp:391:46 (cloud-blockstore-vhost-server-ut+0x1e9def9) (BuildId: 8e691127638c86e2d0906170455404ea4e367474) #1 NCloud::NBlockStore::NVHostServer::(anonymous namespace)::TAioBackend::Start()::$_0::operator()() const /cloud/blockstore/vhost-server/backend_aio.cpp:229:45 (cloud-blockstore-vhost-server-ut+0x1e9d5a5) (BuildId: 8e691127638c86e2d0906170455404ea4e367474) #2 decltype(std::declval()()) std::__y1::__invoke[abi:v15000](NCloud::NBlockStore::NVHostServer::(anonymous namespace)::TAioBackend::Start()::$_0&&) /contrib/libs/cxxsupp/libcxx/include/__functional/invoke.h:403:23 (cloud-blockstore-vhost-server-ut+0x1e9d505) (BuildId: 8e691127638c86e2d0906170455404ea4e367474) #3 void std::__y1::__thread_execute[abi:v15000]>, NCloud::NBlockStore::NVHostServer::(anonymous namespace)::TAioBackend::Start()::$_0>(std::__y1::tuple>, NCloud::NBlockStore::NVHostServer::(anonymous namespace)::TAioBackend::Start()::$_0>&, std::__y1::__tuple_indices<>) /contrib/libs/cxxsupp/libcxx/include/thread:284:5 (cloud-blockstore-vhost-server-ut+0x1e9d3cd) (BuildId: 8e691127638c86e2d0906170455404ea4e367474) #4 void* std::__y1::__thread_proxy[abi:v15000]>, NCloud::NBlockStore::NVHostServer::(anonymous namespace)::TAioBackend::Start()::$_0>>(void*) /contrib/libs/cxxsupp/libcxx/include/thread:295:5 (cloud-blockstore-vhost-server-ut+0x1e9cb72) (BuildId: 8e691127638c86e2d0906170455404ea4e367474) Previous write of size 8 at 0x7b1c000070b0 by thread T7: #0 calloc /contrib/libs/clang16-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:712:5 (cloud-blockstore-vhost-server-ut+0x1fd12e1) (BuildId: 8e691127638c86e2d0906170455404ea4e367474) #1 NCloud::NBlockStore::NVHostServer::PrepareIO(TLog&, TVector> const&, vhd_io*, TVector>&, unsigned long) /cloud/blockstore/vhost-server/request_aio.cpp:152:43 (cloud-blockstore-vhost-server-ut+0x1ee9700) (BuildId: 8e691127638c86e2d0906170455404ea4e367474) #2 NCloud::NBlockStore::NVHostServer::(anonymous namespace)::TAioBackend::PrepareBatch(vhd_request_queue*, TVector>&, unsigned long) /cloud/blockstore/vhost-server/backend_aio.cpp:320:9 (cloud-blockstore-vhost-server-ut+0x1ea5e52) (BuildId: 8e691127638c86e2d0906170455404ea4e367474) #3 NCloud::NBlockStore::NVHostServer::(anonymous namespace)::TAioBackend::ProcessQueue(unsigned int, vhd_request_queue*, NCloud::NBlockStore::NVHostServer::TStats&) /cloud/blockstore/vhost-server/backend_aio.cpp:256:32 (cloud-blockstore-vhost-server-ut+0x1e9a3f7) (BuildId: 8e691127638c86e2d0906170455404ea4e367474) #4 NCloud::NBlockStore::NVHostServer::(anonymous namespace)::TServer::QueueThreadFunc(unsigned int) /cloud/blockstore/vhost-server/server.cpp:252:18 (cloud-blockstore-vhost-server-ut+0x1f36661) (BuildId: 8e691127638c86e2d0906170455404ea4e367474) #5 NCloud::NBlockStore::NVHostServer::(anonymous namespace)::TServer::Start(NCloud::NBlockStore::NVHostServer::TOptions const&)::$_0::operator()() const /cloud/blockstore/vhost-server/server.cpp:173:47 (cloud-blockstore-vhost-server-ut+0x1f362ed) (BuildId: 8e691127638c86e2d0906170455404ea4e367474) #6 decltype(std::declval()()) std::__y1::__invoke[abi:v15000](NCloud::NBlockStore::NVHostServer::(anonymous namespace)::TServer::Start(NCloud::NBlockStore::NVHostServer::TOptions const&)::$_0&&) /contrib/libs/cxxsupp/libcxx/include/__functional/invoke.h:403:23 (cloud-blockstore-vhost-server-ut+0x1f36235) (BuildId: 8e691127638c86e2d0906170455404ea4e367474) ``` This is since TSAN doesn't understand that request allocated in VHOST thread is later read in AIO thread when completion happens. This patch adds explicit tsan annotations to mitigate these false positives --- cloud/blockstore/vhost-server/backend_aio.cpp | 5 +++++ cloud/blockstore/vhost-server/request_aio.cpp | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/cloud/blockstore/vhost-server/backend_aio.cpp b/cloud/blockstore/vhost-server/backend_aio.cpp index 15ba333561d..89f01e2048b 100644 --- a/cloud/blockstore/vhost-server/backend_aio.cpp +++ b/cloud/blockstore/vhost-server/backend_aio.cpp @@ -8,6 +8,8 @@ #include #include +#include + #include #include @@ -362,7 +364,9 @@ void TAioBackend::CompletionThreadFunc() for (int i = 0; i != ret; ++i) { if (events[i].data) { auto* req = static_cast(events[i].data); + NSan::Acquire(req); iocb* sub = events[i].obj; + NSan::Acquire(sub); vhd_bdev_io_result result = VHD_BDEV_SUCCESS; @@ -386,6 +390,7 @@ void TAioBackend::CompletionThreadFunc() } auto* req = static_cast(events[i].obj); + NSan::Acquire(req); vhd_bdev_io_result result = VHD_BDEV_SUCCESS; auto* bio = vhd_get_bdev_io(req->Io); diff --git a/cloud/blockstore/vhost-server/request_aio.cpp b/cloud/blockstore/vhost-server/request_aio.cpp index 1635a6f4725..900dc93e749 100644 --- a/cloud/blockstore/vhost-server/request_aio.cpp +++ b/cloud/blockstore/vhost-server/request_aio.cpp @@ -3,6 +3,7 @@ #include #include +#include #include @@ -81,11 +82,13 @@ void PrepareCompoundIO( sreq->data = req; batch.push_back(sreq); + NSan::Release(sreq); ptr += count; totalBytes -= count; deviceOffset = 0; } + NSan::Release(req); } } // namespace @@ -198,6 +201,7 @@ void PrepareIO( STORAGE_DEBUG("Prepared IO request with addr: %p", req); batch.push_back(req); + NSan::Release(req); } } // namespace NCloud::NBlockStore::NVHostServer From ff0d3a0bfc550fb82869b0251fb4a00f1139f220 Mon Sep 17 00:00:00 2001 From: Evgeny Budilovsky Date: Tue, 2 Apr 2024 19:36:48 +0300 Subject: [PATCH 4/4] NEBDUTY-965: avoid data race when reading WaitMode from rdma client config Rdma client config can be modified during initial hand shake that's why CM thread overwrites it by original config when Qp created in `TClientEndpoint::CreateQP` thread sanitizer reports errors since this Config.WaitMode is used in completion poller thread. To avoid this error WaitMode is read only once during the initialization and of the endpoint and we don't modify it any more Thread sanitizer error looks like that ``` WARNING: ThreadSanitizer: data race (pid=2354960) Write of size 8 at 0x7b5c00000118 by thread T2: #0 __tsan_memcpy contrib/libs/clang16-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:3165:3 (cloud-blockstore-libs-rdma-impl-ut+0x22f79f4) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #1 NCloud::NBlockStore::NRdma::(anonymous namespace)::TClientEndpoint::CreateQP() cloud/blockstore/libs/rdma/impl/client.cpp:591:16 (cloud-blockstore-libs-rdma-impl-ut+0x49e974a) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #2 NCloud::NBlockStore::NRdma::(anonymous namespace)::TClient::BeginConnect(NCloud::NBlockStore::NRdma::(anonymous namespace)::TClientEndpoint*) cloud/blockstore/libs/rdma/impl/client.cpp:1876:19 (cloud-blockstore-libs-rdma-impl-ut+0x49e85dc) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #3 NCloud::NBlockStore::NRdma::(anonymous namespace)::TClient::HandleConnectionEvent(std::__y1::unique_ptr) cloud/blockstore/libs/rdma/impl/client.cpp:1736:13 (cloud-blockstore-libs-rdma-impl-ut+0x49bbfb0) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #4 non-virtual thunk to NCloud::NBlockStore::NRdma::(anonymous namespace)::TClient::HandleConnectionEvent(std::__y1::unique_ptr) cloud/blockstore/libs/rdma/impl/client.cpp (cloud-blockstore-libs-rdma-impl-ut+0x49bc761) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #5 NCloud::NBlockStore::NRdma::(anonymous namespace)::TConnectionPoller::HandleConnectionEvents() cloud/blockstore/libs/rdma/impl/client.cpp:1282:27 (cloud-blockstore-libs-rdma-impl-ut+0x49d3dca) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #6 NCloud::NBlockStore::NRdma::(anonymous namespace)::TConnectionPoller::ThreadProc() cloud/blockstore/libs/rdma/impl/client.cpp:1254:25 (cloud-blockstore-libs-rdma-impl-ut+0x49d3870) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #7 non-virtual thunk to NCloud::NBlockStore::NRdma::(anonymous namespace)::TConnectionPoller::ThreadProc() cloud/blockstore/libs/rdma/impl/client.cpp (cloud-blockstore-libs-rdma-impl-ut+0x49d39d9) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #8 void* (anonymous namespace)::ThreadProcWrapper(void*) util/system/thread.cpp:383:45 (cloud-blockstore-libs-rdma-impl-ut+0x2680796) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #9 (anonymous namespace)::TPosixThread::ThreadProxy(void*) util/system/thread.cpp:244:20 (cloud-blockstore-libs-rdma-impl-ut+0x268b82e) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) Previous read of size 4 at 0x7b5c00000118 by thread T1: #0 NCloud::NBlockStore::NRdma::(anonymous namespace)::TClientEndpoint::AbortRequests() cloud/blockstore/libs/rdma/impl/client.cpp:814:16 (cloud-blockstore-libs-rdma-impl-ut+0x49c4494) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #1 NCloud::NBlockStore::NRdma::(anonymous namespace)::TCompletionPoller::HandlePollEvent(epoll_event const&) cloud/blockstore/libs/rdma/impl/client.cpp:1428:31 (cloud-blockstore-libs-rdma-impl-ut+0x49c4042) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #2 NCloud::NBlockStore::NRdma::(anonymous namespace)::TCompletionPoller::HandlePollEvents() cloud/blockstore/libs/rdma/impl/client.cpp:1446:17 (cloud-blockstore-libs-rdma-impl-ut+0x49c3736) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #3 void NCloud::NBlockStore::NRdma::(anonymous namespace)::TCompletionPoller::Execute<(NCloud::NBlockStore::NRdma::EWaitMode)0>() cloud/blockstore/libs/rdma/impl/client.cpp:1530:21 (cloud-blockstore-libs-rdma-impl-ut+0x49c3341) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #4 NCloud::NBlockStore::NRdma::(anonymous namespace)::TCompletionPoller::ThreadProc() cloud/blockstore/libs/rdma/impl/client.cpp:1398:17 (cloud-blockstore-libs-rdma-impl-ut+0x49c1c18) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #5 non-virtual thunk to NCloud::NBlockStore::NRdma::(anonymous namespace)::TCompletionPoller::ThreadProc() cloud/blockstore/libs/rdma/impl/client.cpp (cloud-blockstore-libs-rdma-impl-ut+0x49c1cf9) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #6 void* (anonymous namespace)::ThreadProcWrapper(void*) util/system/thread.cpp:383:45 (cloud-blockstore-libs-rdma-impl-ut+0x2680796) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) #7 (anonymous namespace)::TPosixThread::ThreadProxy(void*) util/system/thread.cpp:244:20 (cloud-blockstore-libs-rdma-impl-ut+0x268b82e) (BuildId: 49c919960fac17dc20567e23b8a866c9294bf128) ``` --- cloud/blockstore/libs/rdma/impl/client.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/cloud/blockstore/libs/rdma/impl/client.cpp b/cloud/blockstore/libs/rdma/impl/client.cpp index c4484749714..0b4c9df7157 100644 --- a/cloud/blockstore/libs/rdma/impl/client.cpp +++ b/cloud/blockstore/libs/rdma/impl/client.cpp @@ -389,6 +389,7 @@ class TClientEndpoint final // config might be adjusted during initial handshake TClientConfigPtr OriginalConfig; TClientConfig Config; + const EWaitMode WaitMode; bool ResetConfig = false; TCompletionPoller* Poller = nullptr; @@ -524,6 +525,7 @@ TClientEndpoint::TClientEndpoint( , Reconnect(config->MaxReconnectDelay) , OriginalConfig(std::move(config)) , Config(*OriginalConfig) + , WaitMode(Config.WaitMode) { // user data attached to connection events Connection->context = this; @@ -769,14 +771,14 @@ void TClientEndpoint::SendRequest( Counters->RequestEnqueued(); InputRequests.Enqueue(std::move(req)); - if (Config.WaitMode == EWaitMode::Poll) { + if (WaitMode == EWaitMode::Poll) { RequestEvent.Set(); } } bool TClientEndpoint::HandleInputRequests() { - if (Config.WaitMode == EWaitMode::Poll) { + if (WaitMode == EWaitMode::Poll) { RequestEvent.Clear(); } @@ -811,7 +813,7 @@ bool TClientEndpoint::AbortRequests() noexcept { bool ret = false; - if (Config.WaitMode == EWaitMode::Poll) { + if (WaitMode == EWaitMode::Poll) { DisconnectEvent.Clear(); } @@ -856,7 +858,7 @@ bool TClientEndpoint::HandleCompletionEvents() { ibv_cq* cq = CompletionQueue.get(); - if (Config.WaitMode == EWaitMode::Poll) { + if (WaitMode == EWaitMode::Poll) { Verbs->GetCompletionEvent(cq); Verbs->AckCompletionEvents(cq, 1); Verbs->RequestCompletionEvent(cq, 0); @@ -1113,7 +1115,7 @@ void TClientEndpoint::SetError() noexcept RDMA_ERROR("flush error: " << e.what()); } - if (Config.WaitMode == EWaitMode::Poll) { + if (WaitMode == EWaitMode::Poll) { DisconnectEvent.Set(); } }