From: Remi Gacogne Date: Wed, 2 Dec 2015 10:43:37 +0000 (+0100) Subject: Protect dnsdist IDState and query ring with a RW lock X-Git-Tag: dnsdist-1.0.0-alpha1~146^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F2846%2Fhead;p=thirdparty%2Fpdns.git 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 544838eae6..0b99313424 100644 --- a/pdns/dnsdist-lua.cc +++ b/pdns/dnsdist-lua.cc @@ -5,6 +5,7 @@ #include "sodcrypto.hh" #include "base64.hh" #include +#include "lock.hh" using std::thread; @@ -656,9 +657,12 @@ vector> setupLua(bool client, const std::string& confi g_lua.writeFunction("topClients", [](unsigned int top) { map 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> rcounts; for(const auto& c : counts) @@ -683,6 +687,7 @@ vector> setupLua(bool client, const std::string& confi map 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> 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 "<> rcounts; diff --git a/pdns/dnsdist-lua2.cc b/pdns/dnsdist-lua2.cc index 21c627122c..be0069f1b3 100644 --- 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 #include @@ -69,6 +70,7 @@ map exceedQueryGen(int rate, int seconds, std::function #include @@ -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 3c05ffdad8..5f78dceda2 100644 --- a/pdns/dnsdist.cc +++ b/pdns/dnsdist.cc @@ -38,6 +38,7 @@ #include "dnsrulactions.hh" #include #include +#include "lock.hh" #include /* Known sins: @@ -156,7 +157,12 @@ void* responderThread(std::shared_ptr 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 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 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 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 7d358fbaec..a778dd58d8 100644 --- 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 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 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 1b5cc81bc2..ce3e840ada 100644 --- 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 index 0000000000..718008b923 --- /dev/null +++ b/pdns/dnsdistdist/lock.hh @@ -0,0 +1 @@ +../lock.hh \ No newline at end of file