Use sequential per-dispatch message IDs for TCP

TCP dispentries no longer use the global QID hash table at all.
Responses are matched by scanning disp->active, and sequential
per-dispatch IDs (bounded by the pipelining limit) are unique
within a single dispatch by construction.  Since TCP delivers
only data we asked for on a specific connection, the per-peer
uniqueness that the global table enforced was never actually
needed for TCP.

DNS_DISPATCHOPT_FIXEDID is plumbed through dns_request_createraw
-> get_dispatch -> dns_dispatch_createtcp so FIXEDID TCP requests
always get a fresh isolated dispatch — the caller-supplied ID
then cannot collide with any other in-flight query either.

diff --git a/bin/tests/system/masterformat/ns2/named.args b/bin/tests/system/masterformat/ns2/named.args
new file mode 100644 (file)
index 0000000..938e73b
--- /dev/null
+++ b/bin/tests/system/masterformat/ns2/named.args
@@ -0,0 +1 @@
+-D masterformat-ns2 -m record -c named.conf -d 99 -g -T maxcachesize=2097152 -T tcppipelining=1

diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c
index 74209ff50b6e5b95ef0fc0a51edc09f8ac697636..bef5165a5168156bcfea0b6830c46948a24bb6b7 100644 (file)
--- a/lib/dns/dispatch.c
+++ b/lib/dns/dispatch.c
@@ -134,6 +134,8 @@ struct dns_dispatch {
        dns_dispatchstate_t state;
        dns_dispatchtype_t disptype;
 
+       dns_messageid_t nextid; /*%< next sequential QID for TCP */
+
        bool reading;
 
        dns_displist_t pending;
@@ -651,12 +653,9 @@ tcp_recv_oldest(dns_dispatch_t *disp, dns_dispentry_t **respp) {
        return ISC_R_NOTFOUND;
 }
 
-/*
- * NOTE: Must be RCU read locked!
- */
 static isc_result_t
 tcp_recv_success(dns_dispatch_t *disp, isc_region_t *region,
-                isc_sockaddr_t *peer, dns_dispentry_t **respp) {
+                dns_dispentry_t **respp) {
        isc_buffer_t source;
        dns_messageid_t id;
        unsigned int flags;
@@ -692,37 +691,24 @@ tcp_recv_success(dns_dispatch_t *disp, isc_region_t *region,
        }
 
        /*
-        * We have a valid response; find the associated dispentry object
-        * and call the caller back.
+        * We have a valid response; find the associated dispentry by
+        * scanning disp->active.  With sequential IDs and a bounded
+        * pipelining limit this is a short linear scan.
         */
-       dns_dispentry_t key = {
-               .id = id,
-               .peer = *peer,
-               .port = isc_sockaddr_getport(&disp->local),
-       };
-       struct cds_lfht_iter iter;
-       cds_lfht_lookup(disp->mgr->qids, qid_hash(&key), qid_match, &key,
-                       &iter);
-
-       dns_dispentry_t *resp = cds_lfht_entry(cds_lfht_iter_get_node(&iter),
-                                              dns_dispentry_t, ht_node);
-
-       /* Skip responses that are not ours */
-       if (resp != NULL && resp->disp == disp) {
-               if (!resp->reading) {
-                       /*
-                        * We already got a message for this QID and weren't
-                        * expecting any more.
-                        */
-                       result = ISC_R_UNEXPECTED;
-               } else {
-                       *respp = resp;
+       dns_dispentry_t *resp = NULL;
+       ISC_LIST_FOREACH(disp->active, r, alink) {
+               if (r->id == id) {
+                       resp = r;
+                       break;
                }
+       }
+
+       if (resp != NULL) {
+               *respp = resp;
        } else {
                result = ISC_R_NOTFOUND;
        }
-       dispatch_log(disp, ISC_LOG_DEBUG(90),
-                    "search for response in hashtable: %s",
+       dispatch_log(disp, ISC_LOG_DEBUG(90), "search for response: %s",
                     isc_result_totext(result));
 
        return result;
@@ -812,7 +798,7 @@ tcp_recv(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region,
                break;
        case ISC_R_SUCCESS:
                /* We got an answer */
-               result = tcp_recv_success(disp, region, &peer, &resp);
+               result = tcp_recv_success(disp, region, &resp);
                break;
 
        default:
@@ -1255,6 +1241,7 @@ dispatch_createtcp(dns_dispatchmgr_t *mgr, const isc_sockaddr_t *localaddr,
        dispatch_allocate(mgr, isc_socktype_tcp, tid, &disp);
 
        disp->disptype = disptype;
+       disp->nextid = isc_random16();
        disp->options = options;
        disp->peer = *destaddr;
        if (transport != NULL) {
@@ -1507,35 +1494,52 @@ dns_dispatch_add(dns_dispatch_t *disp, isc_loop_t *loop,
        }
 
        isc_result_t result = ISC_R_NOMORE;
-       size_t i = 0;
        rcu_read_lock();
-       do {
+
+       if (disp->socktype == isc_socktype_tcp) {
                /*
-                * Try somewhat hard to find a unique ID. Start with
-                * a random number unless DNS_DISPATCHOPT_FIXEDID is set,
-                * in which case we start with the ID passed in via *idp.
+                * TCP dispentries don't use the global QID hash table.
+                * Responses are matched by scanning disp->active, and
+                * sequential per-dispatch IDs (bounded by the pipelining
+                * limit) are guaranteed to be unique within the dispatch.
+                * FIXEDID TCP dispatches are always fresh and isolated
+                * (see dns_dispatch_createtcp), so the caller-supplied ID
+                * can't collide either.
                 */
                resp->id = ((options & DNS_DISPATCHOPT_FIXEDID) != 0)
                                   ? *idp
-                                  : (dns_messageid_t)isc_random16();
-
-               struct cds_lfht_node *node =
-                       cds_lfht_add_unique(disp->mgr->qids, qid_hash(resp),
-                                           qid_match, resp, &resp->ht_node);
-
-               if (node != &resp->ht_node) {
-                       if ((options & DNS_DISPATCHOPT_FIXEDID) != 0) {
-                               /*
-                                * When using fixed ID, we either must
-                                * use it or fail
-                                */
-                               goto fail;
+                                  : disp->nextid++;
+               result = ISC_R_SUCCESS;
+       } else {
+               size_t i = 0;
+               do {
+                       /*
+                        * Try somewhat hard to find a unique random ID
+                        * (or use the fixed ID if DNS_DISPATCHOPT_FIXEDID
+                        * is set).
+                        */
+                       resp->id = ((options & DNS_DISPATCHOPT_FIXEDID) != 0)
+                                          ? *idp
+                                          : (dns_messageid_t)isc_random16();
+
+                       struct cds_lfht_node *node = cds_lfht_add_unique(
+                               disp->mgr->qids, qid_hash(resp), qid_match,
+                               resp, &resp->ht_node);
+
+                       if (node != &resp->ht_node) {
+                               if ((options & DNS_DISPATCHOPT_FIXEDID) != 0) {
+                                       /*
+                                        * When using fixed ID, we either
+                                        * must use it or fail.
+                                        */
+                                       goto fail;
+                               }
+                       } else {
+                               result = ISC_R_SUCCESS;
+                               break;
                        }
-               } else {
-                       result = ISC_R_SUCCESS;
-                       break;
-               }
-       } while (i++ < QID_MAX_TRIES);
+               } while (i++ < QID_MAX_TRIES);
+       }
 fail:
        if (result != ISC_R_SUCCESS) {
                isc_mem_put(disp->mctx, resp, sizeof(*resp));
@@ -1764,8 +1768,6 @@ tcp_dispentry_cancel(dns_dispentry_t *resp, isc_result_t result) {
 
        dec_stats(disp->mgr, dns_resstatscounter_dispreqtcp);
 
-       (void)cds_lfht_del(disp->mgr->qids, &resp->ht_node);
-
        resp->state = DNS_DISPATCHSTATE_CANCELED;
 
 unlock:

diff --git a/lib/dns/request.c b/lib/dns/request.c
index 4634333aa6841a251efc3de7412b021dae4b850d..18f24661dbaae31c87590e4bf9683c032ee48e8e 100644 (file)
--- a/lib/dns/request.c
+++ b/lib/dns/request.c
@@ -336,10 +336,10 @@ isblackholed(dns_dispatchmgr_t *dispatchmgr, const isc_sockaddr_t *destaddr) {
 static isc_result_t
 tcp_dispatch(dns_requestmgr_t *requestmgr, const isc_sockaddr_t *srcaddr,
             const isc_sockaddr_t *destaddr, dns_transport_t *transport,
-            dns_dispatch_t **dispatchp) {
-       return dns_dispatch_createtcp(requestmgr->dispatchmgr, srcaddr,
-                                     destaddr, transport,
-                                     DNS_DISPATCHTYPE_REQUEST, 0, dispatchp);
+            unsigned int dispopt, dns_dispatch_t **dispatchp) {
+       return dns_dispatch_createtcp(
+               requestmgr->dispatchmgr, srcaddr, destaddr, transport,
+               DNS_DISPATCHTYPE_REQUEST, dispopt, dispatchp);
 }
 
 static isc_result_t
@@ -374,12 +374,13 @@ udp_dispatch(dns_requestmgr_t *requestmgr, const isc_sockaddr_t *srcaddr,
 static isc_result_t
 get_dispatch(bool tcp, dns_requestmgr_t *requestmgr,
             const isc_sockaddr_t *srcaddr, const isc_sockaddr_t *destaddr,
-            dns_transport_t *transport, dns_dispatch_t **dispatchp) {
+            dns_transport_t *transport, unsigned int dispopt,
+            dns_dispatch_t **dispatchp) {
        isc_result_t result;
 
        if (tcp) {
                result = tcp_dispatch(requestmgr, srcaddr, destaddr, transport,
-                                     dispatchp);
+                                     dispopt, dispatchp);
        } else {
                result = udp_dispatch(requestmgr, srcaddr, destaddr, dispatchp);
        }
@@ -449,14 +450,14 @@ dns_request_createraw(dns_requestmgr_t *requestmgr, isc_buffer_t *msgbuf,
        isc_buffer_allocate(mctx, &request->query, r.length + (tcp ? 2 : 0));
        CHECK(isc_buffer_copyregion(request->query, &r));
 
-       CHECK(get_dispatch(tcp, requestmgr, srcaddr, destaddr, transport,
-                          &request->dispatch));
-
        if ((options & DNS_REQUESTOPT_FIXEDID) != 0) {
                id = (r.base[0] << 8) | r.base[1];
                dispopt |= DNS_DISPATCHOPT_FIXEDID;
        }
 
+       CHECK(get_dispatch(tcp, requestmgr, srcaddr, destaddr, transport,
+                          dispopt, &request->dispatch));
+
        CHECK(dns_dispatch_add(request->dispatch, loop, dispopt,
                               request->connect_timeout, request->timeout,
                               destaddr, transport, tlsctx_cache, req_connected,
@@ -560,7 +561,7 @@ dns_request_create(dns_requestmgr_t *requestmgr, dns_message_t *message,
        CHECK(dns_message_settsigkey(message, request->tsigkey));
 
 again:
-       CHECK(get_dispatch(tcp, requestmgr, srcaddr, destaddr, transport,
+       CHECK(get_dispatch(tcp, requestmgr, srcaddr, destaddr, transport, 0,
                           &request->dispatch));
 
        CHECK(dns_dispatch_add(request->dispatch, loop, 0,