From 0e41337bff27ebd9813b04662d74672c4a160087 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Wed, 2 Dec 2015 11:43:37 +0100 Subject: [PATCH] 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. --- pdns/dnsdist-lua.cc | 13 +++++-- pdns/dnsdist-lua2.cc | 2 + pdns/dnsdist-tcp.cc | 6 ++- pdns/dnsdist.cc | 71 ++++++++++++++++++++++-------------- pdns/dnsdist.hh | 8 +++- pdns/dnsdistdist/Makefile.am | 1 + pdns/dnsdistdist/lock.hh | 1 + 7 files changed, 68 insertions(+), 34 deletions(-) create mode 120000 pdns/dnsdistdist/lock.hh diff --git a/pdns/dnsdist-lua.cc b/pdns/dnsdist-lua.cc index 544838eae..0b9931342 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 21c627122..be0069f1b 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 3c05ffdad..5f78dceda 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 7d358fbae..a778dd58d 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 1b5cc81bc..ce3e840ad 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 000000000..718008b92 --- /dev/null +++ b/pdns/dnsdistdist/lock.hh @@ -0,0 +1 @@ +../lock.hh \ No newline at end of file -- 2.40.0