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 e507c0f889e5970039968a16399d9c37e569f9a3..c871d81ba06e93b8fec616f2e8634bed392534c8 100644 (file)
--- a/pdns/xsk.cc
+++ b/pdns/xsk.cc
@@ -1213,33 +1213,16 @@ std::vector<pollfd> getPollFdsForWorker(XskWorker& info)
   return fds;
 }
 
-void XskWorker::fillUniqueEmptyOffset()
-{
-  auto frames = sharedEmptyFrameOffset->lock();
-  const auto moveSize = std::min(static_cast<size_t>(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<XskPacket> 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 8d2b57d2d95c2379e4b18b2edf0c48520d050035..ca6e65ca04fd9c11e630b9cab496d53a938b7ba6 100644 (file)
--- 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<LockGuarded<vector<uint64_t>>> sharedEmptyFrameOffset;
-  // list of frames that we own, used to generate new packets (health-check)
-  vector<uint64_t> 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<XskPacket> getEmptyFrame();
 };
 std::vector<pollfd> getPollFdsForWorker(XskWorker& info);