From: Remi Gacogne Date: Thu, 4 Jul 2024 15:16:54 +0000 (+0200) Subject: dnsdist: Fix a race in the XSK/AF_XDP backend handling code X-Git-Tag: rec-5.2.0-alpha1~179^2~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b81c7e42fcdec03a3541329a5704487d5c9e925a;p=thirdparty%2Fpdns.git dnsdist: Fix a race in the XSK/AF_XDP backend handling code For performance reasons we used to keep a local list of available frames in our `XskWorker` object, like we are doing in the `XskSocket` one, to avoid having to go to the shared list which is protected by a lock. Unfortunately, while it works well for the `XskSocket` because it is accessed by a single `XskRouter` thread, the `XskWorker` object can be accessed by multiple threads at once: `XskResponderThread`, `responderThread`, `XskClientThread` and `XskRouter`. Most of the time these threads do not acquire nor release frames to the local list, but `responderThread` does acquire one when a response frame is punted to the regular networking stack, and all of them release frames when an unexpected condition occurs, for example when a queue is full. This leads to memory corruption and to a crash. This commit gets rid of the local list of frames in the `XskWorker` object, acquiring and releasing them to the shared list instead, since performance in these cases is likely not as critical. If it turns out to be too slow, we can look into caching a few frames in a thread-local list, but then we need to be careful not to hold on them indefinitely which might be tricky. --- diff --git a/pdns/xsk.cc b/pdns/xsk.cc index e507c0f889..c871d81ba0 100644 --- a/pdns/xsk.cc +++ b/pdns/xsk.cc @@ -1213,33 +1213,16 @@ std::vector getPollFdsForWorker(XskWorker& info) return fds; } -void XskWorker::fillUniqueEmptyOffset() -{ - auto frames = sharedEmptyFrameOffset->lock(); - const auto moveSize = std::min(static_cast(32), frames->size()); - if (moveSize > 0) { - // NOLINTNEXTLINE(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions) - uniqueEmptyFrameOffset.insert(uniqueEmptyFrameOffset.end(), std::make_move_iterator(frames->end() - moveSize), std::make_move_iterator(frames->end())); - frames->resize(frames->size() - moveSize); - } -} - std::optional XskWorker::getEmptyFrame() { - if (!uniqueEmptyFrameOffset.empty()) { - auto offset = uniqueEmptyFrameOffset.back(); - uniqueEmptyFrameOffset.pop_back(); - // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) - return XskPacket(offset + umemBufBase, 0, frameSize); - } - fillUniqueEmptyOffset(); - if (!uniqueEmptyFrameOffset.empty()) { - auto offset = uniqueEmptyFrameOffset.back(); - uniqueEmptyFrameOffset.pop_back(); - // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) - return XskPacket(offset + umemBufBase, 0, frameSize); + auto frames = sharedEmptyFrameOffset->lock(); + if (frames->empty()) { + return std::nullopt; } - return std::nullopt; + auto offset = frames->back(); + frames->pop_back(); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) + return XskPacket(offset + umemBufBase, 0, frameSize); } void XskWorker::markAsFree(const XskPacket& packet) @@ -1248,7 +1231,10 @@ void XskWorker::markAsFree(const XskPacket& packet) #ifdef DEBUG_UMEM checkUmemIntegrity(__PRETTY_FUNCTION__, __LINE__, offset, {UmemEntryStatus::Status::Received, UmemEntryStatus::Status::TXQueue}, UmemEntryStatus::Status::Free); #endif /* DEBUG_UMEM */ - uniqueEmptyFrameOffset.push_back(offset); + { + auto frames = sharedEmptyFrameOffset->lock(); + frames->push_back(offset); + } } uint32_t XskPacket::getFlags() const noexcept diff --git a/pdns/xsk.hh b/pdns/xsk.hh index 8d2b57d2d9..ca6e65ca04 100644 --- a/pdns/xsk.hh +++ b/pdns/xsk.hh @@ -298,8 +298,6 @@ public: uint8_t* umemBufBase{nullptr}; // list of frames that are shared with the XskRouter std::shared_ptr>> sharedEmptyFrameOffset; - // list of frames that we own, used to generate new packets (health-check) - vector uniqueEmptyFrameOffset; const size_t frameSize{XskSocket::getFrameSize()}; FDWrapper workerWaker; FDWrapper xskSocketWaker; @@ -319,10 +317,7 @@ public: void cleanWorkerNotification() const noexcept; void cleanSocketNotification() const noexcept; [[nodiscard]] uint64_t frameOffset(const XskPacket& packet) const noexcept; - // reap empty umem entry from sharedEmptyFrameOffset into uniqueEmptyFrameOffset - void fillUniqueEmptyOffset(); - // look for an empty umem entry in uniqueEmptyFrameOffset - // then sharedEmptyFrameOffset if needed + // get an empty umem entry from sharedEmptyFrameOffset std::optional getEmptyFrame(); }; std::vector getPollFdsForWorker(XskWorker& info);