Protect dnsdist IDState and query ring with a RW lock

The IDState issue is mainly origFD, modified by maintThread on timeout while used by the others.
upStatus and availability in DownstreamState are also causing complaints from helgrind / TSAN,
but I believe we can live with racy status and availability.

diff --git a/pdns/dnsdist-lua.cc b/pdns/dnsdist-lua.cc
index 544838eae6a0bbafa7ea1c6817fc6d80ba0e9c84..0b99313424547f7122bccd0768587cec35b5eec2 100644 (file)
--- a/pdns/dnsdist-lua.cc
+++ b/pdns/dnsdist-lua.cc
@@ -5,6 +5,7 @@
 #include "sodcrypto.hh"
 #include "base64.hh"
 #include <fstream>
+#include "lock.hh"
 
 using std::thread;
 
@@ -656,9 +657,12 @@ vector<std::function<void(void)>> setupLua(bool client, const std::string& confi
   g_lua.writeFunction("topClients", [](unsigned int top) {
       map<ComboAddress, int,ComboAddress::addressOnlyLessThan > counts;
       unsigned int total=0;
-      for(const auto& c : g_rings.queryRing) {
-       counts[c.requestor]++;
-       total++;
+      {
+        ReadLock rl(&g_rings.queryLock);
+        for(const auto& c : g_rings.queryRing) {
+          counts[c.requestor]++;
+          total++;
+        }
       }
       vector<pair<int, ComboAddress>> rcounts;
       for(const auto& c : counts) 
@@ -683,6 +687,7 @@ vector<std::function<void(void)>> setupLua(bool client, const std::string& confi
       map<DNSName, int> counts;
       unsigned int total=0;
       if(!labels) {
+       ReadLock rl(&g_rings.queryLock);
        for(const auto& a : g_rings.queryRing) {
          counts[a.name]++;
          total++;
@@ -690,12 +695,12 @@ vector<std::function<void(void)>> setupLua(bool client, const std::string& confi
       }
       else {
        unsigned int lab = *labels;
+       ReadLock rl(&g_rings.queryLock);
        for(auto a : g_rings.queryRing) {
          a.name.trimToLabels(lab);
          counts[a.name]++;
          total++;
        }
-
       }
       // cout<<"Looked at "<<total<<" queries, "<<counts.size()<<" different ones"<<endl;
       vector<pair<int, DNSName>> rcounts;

diff --git a/pdns/dnsdist-lua2.cc b/pdns/dnsdist-lua2.cc
index 21c627122c1e29fa87044340be71aa8d6f48d283..be0069f1b3b71f8230dc37a861002456ac04d609 100644 (file)
--- a/pdns/dnsdist-lua2.cc
+++ b/pdns/dnsdist-lua2.cc
@@ -4,6 +4,7 @@
 #include "dolog.hh"
 #include "sodcrypto.hh"
 #include "base64.hh"
+#include "lock.hh"
 #include <map>
 #include <fstream>
 
@@ -69,6 +70,7 @@ map<ComboAddress,int> exceedQueryGen(int rate, int seconds, std::function<void(c
   mintime=cutoff=maxtime;
   cutoff.tv_sec -= seconds;
   
+  ReadLock rl(&g_rings.queryLock);
   for(const auto& c : g_rings.queryRing) {
     if(seconds && c.when < cutoff)
       continue;

diff --git a/pdns/dnsdist-tcp.cc b/pdns/dnsdist-tcp.cc
index 739ad5200cbae08939e694990d7b0258cf9e38d4..7a93ba7d8360ffb8c971b8b689e2db8b44119299 100644 (file)
--- a/pdns/dnsdist-tcp.cc
+++ b/pdns/dnsdist-tcp.cc
@@ -22,6 +22,7 @@
 
 #include "dnsdist.hh"
 #include "dolog.hh"
+#include "lock.hh"
 #include <thread>
 #include <atomic>
 
@@ -164,7 +165,10 @@ void* tcpClientThread(int pipefd)
        struct timespec now;
        clock_gettime(CLOCK_MONOTONIC, &now);
 
-       g_rings.queryRing.push_back({now,ci.remote,qname,qtype}); // XXX LOCK?!
+       {
+         WriteLock wl(&g_rings.queryLock);
+         g_rings.queryRing.push_back({now,ci.remote,qname,qtype});
+       }
 
        if(localDynBlockNMG->match(ci.remote)) {
          vinfolog("Query from %s dropped because of dynamic block", ci.remote.toStringWithPort());

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index 3c05ffdad83e72dfcb41215ead519e3953d6f576..5f78dceda22f29a30c126e297c70cafb9fe35780 100644 (file)
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -38,6 +38,7 @@
 #include "dnsrulactions.hh"
 #include <grp.h>
 #include <pwd.h>
+#include "lock.hh"
 #include <getopt.h>
 
 /* Known sins:
@@ -156,7 +157,12 @@ void* responderThread(std::shared_ptr<DownstreamState> state)
       continue;
 
     IDState* ids = &state->idStates[dh->id];
-    if(ids->origFD < 0) // duplicate
+    int origFD;
+    {
+      ReadLock rl(&(ids->lock));
+      origFD = ids->origFD;
+    }
+    if(origFD < 0) // duplicate
       continue;
     else
       --state->outstanding;  // you'd think an attacker could game this, but we're using connected socket
@@ -178,14 +184,14 @@ void* responderThread(std::shared_ptr<DownstreamState> state)
     g_stats.responses++;
 
     if(ids->delayMsec && g_delay) {
-      DelayedPacket dp{ids->origFD, string(packet,len), ids->origRemote, ids->origDest};
+      DelayedPacket dp{origFD, string(packet,len), ids->origRemote, ids->origDest};
       g_delay->submit(dp, ids->delayMsec);
     }
     else {
       if(ids->origDest.sin4.sin_family == 0)
-       sendto(ids->origFD, packet, len, 0, (struct sockaddr*)&ids->origRemote, ids->origRemote.getSocklen());
+       sendto(origFD, packet, len, 0, (struct sockaddr*)&ids->origRemote, ids->origRemote.getSocklen());
       else
-       sendfromto(ids->origFD, packet, len, 0, ids->origDest, ids->origRemote);
+       sendfromto(origFD, packet, len, 0, ids->origDest, ids->origRemote);
     }
     double udiff = ids->sentTime.udiff();
     vinfolog("Got answer from %s, relayed to %s, took %f usec", state->remote.toStringWithPort(), ids->origRemote.toStringWithPort(), udiff);
@@ -216,7 +222,11 @@ void* responderThread(std::shared_ptr<DownstreamState> state)
     doAvg(g_stats.latencyAvg10000,   udiff,   10000);
     doAvg(g_stats.latencyAvg1000000, udiff, 1000000);
 
-    ids->origFD = -1;
+    {
+      WriteLock wl(&(ids->lock));
+      if (ids->origFD == origFD)
+        ids->origFD = -1;
+    }
   }
   return 0;
 }
@@ -253,7 +263,7 @@ shared_ptr<DownstreamState> leastOutstanding(const NumberedServerVector& servers
   /* so you might wonder, why do we go through this trouble? The data on which we sort could change during the sort,
      which would suck royally and could even lead to crashes. So first we snapshot on what we sort, and then we sort */
   poss.reserve(servers.size());
-  for(auto& d : servers) {    
+  for(auto& d : servers) {
     if(d.second->isUp()) {
       poss.push_back({make_tuple(d.second->outstanding.load(), d.second->order, d.second->latencyUsec), d.second});
     }
@@ -460,7 +470,10 @@ try
       DNSName qname(packet, len, 12, false, &qtype);
       struct timespec now;
       clock_gettime(CLOCK_MONOTONIC, &now);
-      g_rings.queryRing.push_back({now,remote,qname,qtype}); // XXX LOCK?!
+      {
+        WriteLock wl(&g_rings.queryLock);
+        g_rings.queryRing.push_back({now,remote,qname,qtype});
+      }
       
       if(localDynBlock->match(remote)) {
        vinfolog("Query from %s dropped because of dynamic block", remote.toStringWithPort());
@@ -545,27 +558,30 @@ try
       ss->queries++;
       
       unsigned int idOffset = (ss->idOffset++) % ss->idStates.size();
-      IDState* ids = &ss->idStates[idOffset];
-      
-      if(ids->origFD < 0) // if we are reusing, no change in outstanding
-       ss->outstanding++;
-      else {
-       ss->reuseds++;
-       g_stats.downstreamTimeouts++;
+      {
+        IDState* ids = &ss->idStates[idOffset];
+        WriteLock wl(&ids->lock);
+
+        if(ids->origFD < 0) // if we are reusing, no change in outstanding
+          ss->outstanding++;
+        else {
+          ss->reuseds++;
+          g_stats.downstreamTimeouts++;
+        }
+
+        ids->origFD = cs->udpFD;
+        ids->age = 0;
+        ids->origID = dh->id;
+        ids->origRemote = remote;
+        ids->sentTime.start();
+        ids->qname = qname;
+        ids->qtype = qtype;
+        ids->origDest.sin4.sin_family=0;
+        ids->delayMsec = delayMsec;
+        ids->origFlags = origFlags;
+        HarvestDestinationAddress(&msgh, &ids->origDest);
       }
-      
-      ids->origFD = cs->udpFD;
-      ids->age = 0;
-      ids->origID = dh->id;
-      ids->origRemote = remote;
-      ids->sentTime.start();
-      ids->qname = qname;
-      ids->qtype = qtype;
-      ids->origDest.sin4.sin_family=0;
-      ids->delayMsec = delayMsec;
-      ids->origFlags = origFlags;
-      HarvestDestinationAddress(&msgh, &ids->origDest);
-      
+
       dh->id = idOffset;
       
       len = send(ss->fd, packet, len, 0);
@@ -666,6 +682,7 @@ void* maintThread()
       dss->prev.reuseds.store(dss->reuseds.load());
       
       for(IDState& ids  : dss->idStates) { // timeouts
+        WriteLock wl(&(ids.lock));
         if(ids.origFD >=0 && ids.age++ > 2) {
           ids.age = 0;
           ids.origFD = -1;

diff --git a/pdns/dnsdist.hh b/pdns/dnsdist.hh
index 7d358fbaeccb4efe300cd5c6d3e6bff1f17cec86..a778dd58d8ea702db6e92f1d61a43740e5d99af3 100644 (file)
--- a/pdns/dnsdist.hh
+++ b/pdns/dnsdist.hh
@@ -156,7 +156,7 @@ private:
 
 struct IDState
 {
-  IDState() : origFD(-1), delayMsec(0) { origDest.sin4.sin_family = 0;}
+  IDState() : origFD(-1), delayMsec(0) { origDest.sin4.sin_family = 0; pthread_rwlock_init(&lock, 0);}
   IDState(const IDState& orig)
   {
     origFD = orig.origFD;
@@ -165,6 +165,7 @@ struct IDState
     origDest = orig.origDest;
     delayMsec = orig.delayMsec;
     age.store(orig.age.load());
+    pthread_rwlock_init(&lock, 0);
   }
 
   int origFD;  // set to <0 to indicate this state is empty   // 4
@@ -173,6 +174,7 @@ struct IDState
   ComboAddress origDest;                                      // 28
   StopWatch sentTime;                                         // 16
   DNSName qname;                                              // 80
+  pthread_rwlock_t lock;
   std::atomic<uint16_t> age;                                  // 4
   uint16_t qtype;                                             // 2
   uint16_t origID;                                            // 2
@@ -185,6 +187,7 @@ struct Rings {
   {
     queryRing.set_capacity(10000);
     respRing.set_capacity(10000);
+    pthread_rwlock_init(&queryLock, 0);
   }
   struct Query
   {
@@ -206,9 +209,10 @@ struct Rings {
   };
   boost::circular_buffer<Response> respRing;
   std::mutex respMutex;
+  pthread_rwlock_t queryLock;
 };
 
-extern Rings g_rings; // XXX locking for this is still substandard, queryRing and clientRing need RW lock
+extern Rings g_rings;
 
 struct ClientState
 {

diff --git a/pdns/dnsdistdist/Makefile.am b/pdns/dnsdistdist/Makefile.am
index 1b5cc81bc2cb4867c18199c38b2935cbf9a4b787..ce3e840adaf8e105f1020be23106e2d50b0393b0 100644 (file)
--- a/pdns/dnsdistdist/Makefile.am
+++ b/pdns/dnsdistdist/Makefile.am
@@ -40,6 +40,7 @@ dnsdist_SOURCES = \
        dnswriter.cc dnswriter.hh \
        dolog.hh \
        iputils.cc iputils.hh \
+       lock.hh \
        misc.cc misc.hh \
        htmlfiles.h \
        namespaces.hh \

diff --git a/pdns/dnsdistdist/lock.hh b/pdns/dnsdistdist/lock.hh
new file mode 120000 (symlink)
index 0000000..718008b
--- /dev/null
+++ b/pdns/dnsdistdist/lock.hh
@@ -0,0 +1 @@
+../lock.hh
\ No newline at end of file