From: Vladimír Čunát Date: Fri, 13 Sep 2019 16:26:19 +0000 (+0200) Subject: WIP delay allocation of kr_request::answer X-Git-Tag: v5.2.0~9^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=99e014ac635;p=thirdparty%2Fknot-resolver.git WIP delay allocation of kr_request::answer FIXME: see FIXMEs in diff, document the API change, re-review. --- diff --git a/NEWS b/NEWS index 1672364b9..7f26db736 100644 --- a/NEWS +++ b/NEWS @@ -19,6 +19,11 @@ Bugfixes - avoid an assert() error in stash_rrset() (!1072) - fix emergency cache locking bug introduced in 5.1.3 (!1078) +Incompatible changes +-------------------- +- minor changes in module API; see upgrading guide: + https://knot-resolver.readthedocs.io/en/stable/upgrading.html + Knot Resolver 5.1.3 (2020-09-08) ================================ diff --git a/daemon/lua/kres-gen.lua b/daemon/lua/kres-gen.lua index a689b46dd..3d993dedb 100644 --- a/daemon/lua/kres-gen.lua +++ b/daemon/lua/kres-gen.lua @@ -326,6 +326,7 @@ int knot_pkt_put_rotate(knot_pkt_t *, uint16_t, const knot_rrset_t *, uint16_t, knot_pkt_t *knot_pkt_new(void *, uint16_t, knot_mm_t *); void knot_pkt_free(knot_pkt_t *); int knot_pkt_parse(knot_pkt_t *, unsigned int); +knot_pkt_t *kr_request_ensure_answer(struct kr_request *); struct kr_rplan *kr_resolve_plan(struct kr_request *); knot_mm_t *kr_resolve_pool(struct kr_request *); struct kr_query *kr_rplan_push(struct kr_rplan *, struct kr_query *, const knot_dname_t *, uint16_t, uint16_t); diff --git a/daemon/lua/kres-gen.sh b/daemon/lua/kres-gen.sh index bd5c1fd46..1996b2413 100755 --- a/daemon/lua/kres-gen.sh +++ b/daemon/lua/kres-gen.sh @@ -179,6 +179,7 @@ EOF ## libkres API ${CDEFS} ${LIBKRES} functions <<-EOF # Resolution request + kr_request_ensure_answer kr_resolve_plan kr_resolve_pool # Resolution plan diff --git a/daemon/lua/kres.lua b/daemon/lua/kres.lua index 13021d091..a2cb40613 100644 --- a/daemon/lua/kres.lua +++ b/daemon/lua/kres.lua @@ -893,6 +893,11 @@ ffi.metatype( kr_request_t, { req.vars_ref = ref return var end, + -- Ensure that answer exists and return it; can't fail. + ensure_answer = function (req) + assert(ffi.istype(kr_request_t, req)) + return C.kr_request_ensure_answer(req) + end, }, }) diff --git a/daemon/worker.c b/daemon/worker.c index 6a9f0baf6..e4e1cdc38 100644 --- a/daemon/worker.c +++ b/daemon/worker.c @@ -319,27 +319,13 @@ static struct request_ctx *request_create(struct worker_ctx *worker, static int request_start(struct request_ctx *ctx, knot_pkt_t *query) { assert(query && ctx); - size_t answer_max = KNOT_WIRE_MIN_PKTSIZE; - struct kr_request *req = &ctx->req; - /* source.session can be empty if request was generated by kresd itself */ - struct session *s = ctx->source.session; - if (!s || session_get_handle(s)->type == UV_TCP) { - answer_max = KNOT_WIRE_MAX_PKTSIZE; - } else if (knot_pkt_has_edns(query)) { /* EDNS */ - answer_max = MAX(knot_edns_get_payload(query->opt_rr), - KNOT_WIRE_MIN_PKTSIZE); - } + struct kr_request *req = &ctx->req; req->qsource.size = query->size; if (knot_pkt_has_tsig(query)) { req->qsource.size += query->tsig_wire.len; } - knot_pkt_t *answer = knot_pkt_new(NULL, answer_max, &req->pool); - if (!answer) { /* Failed to allocate answer */ - return kr_error(ENOMEM); - } - knot_pkt_t *pkt = knot_pkt_new(NULL, req->qsource.size, &req->pool); if (!pkt) { return kr_error(ENOMEM); @@ -354,7 +340,7 @@ static int request_start(struct request_ctx *ctx, knot_pkt_t *query) /* Start resolution */ struct worker_ctx *worker = ctx->worker; struct engine *engine = worker->engine; - kr_resolve_begin(req, &engine->resolver, answer); + kr_resolve_begin(req, &engine->resolver); worker->stats.queries += 1; /* Throttle outbound queries only when high pressure */ if (worker->stats.concurrent < QUERY_RATE_THRESHOLD) { diff --git a/daemon/zimport.c b/daemon/zimport.c index b6c5f276d..71a3b1865 100644 --- a/daemon/zimport.c +++ b/daemon/zimport.c @@ -378,7 +378,7 @@ static int zi_rrset_import(zone_import_ctx_t *z_import, knot_rrset_t *rr) qry->zone_cut.key = z_import->key; qry->zone_cut.trust_anchor = z_import->ta; - if (knot_pkt_init_response(request->answer, query) != 0) { + if (knot_pkt_init_response(answer, query) != 0) { goto cleanup; } diff --git a/doc/upgrading.rst b/doc/upgrading.rst index 0e7324037..09ec979df 100644 --- a/doc/upgrading.rst +++ b/doc/upgrading.rst @@ -34,6 +34,19 @@ newer versions when they are released. `DNS Flag Day 2020 `_. Please double-check your firewall, it has to allow DNS traffic on UDP and also TCP port 53. + +5.1 to 5.2 +========== + +Module changes +-------------- +* Reply packet :c:type:`kr_request.answer` + `is not allocated `_ + immediately when the request comes. + See the new :c:func:`kr_request_ensure_answer` function, + wrapped for lua as ``req:ensure_answer()``. + + 5.0 to 5.1 ========== diff --git a/lib/layer/iterate.c b/lib/layer/iterate.c index 76ff12b29..44516e4bc 100644 --- a/lib/layer/iterate.c +++ b/lib/layer/iterate.c @@ -477,7 +477,7 @@ static int process_authority(knot_pkt_t *pkt, struct kr_request *req) static void finalize_answer(knot_pkt_t *pkt, struct kr_request *req) { /* Finalize header */ - knot_pkt_t *answer = req->answer; + knot_pkt_t *answer = kr_request_ensure_answer(req); knot_wire_set_rcode(answer->wire, knot_wire_get_rcode(pkt->wire)); } @@ -902,7 +902,8 @@ static int begin(kr_layer_t *ctx) if (qry->sclass != KNOT_CLASS_IN || (knot_rrtype_is_metatype(qry->stype) /* && qry->stype != KNOT_RRTYPE_ANY hmm ANY seems broken ATM */)) { - knot_wire_set_rcode(ctx->req->answer->wire, KNOT_RCODE_NOTIMPL); + knot_pkt_t *ans = kr_request_ensure_answer(ctx->req); + knot_wire_set_rcode(ans->wire, KNOT_RCODE_NOTIMPL); return KR_STATE_FAIL; } @@ -1061,6 +1062,7 @@ static int resolve(kr_layer_t *ctx, knot_pkt_t *pkt) case KNOT_RCODE_NXDOMAIN: break; /* OK */ case KNOT_RCODE_YXDOMAIN: /* Basically a successful answer; name just doesn't fit. */ + kr_request_ensure_answer(req); knot_wire_set_rcode(req->answer->wire, KNOT_RCODE_YXDOMAIN); break; case KNOT_RCODE_REFUSED: diff --git a/lib/resolve.c b/lib/resolve.c index 0cef2f97c..ba72dc402 100644 --- a/lib/resolve.c +++ b/lib/resolve.c @@ -442,26 +442,6 @@ static int edns_create(knot_pkt_t *pkt, const struct kr_request *req) return knot_pkt_reserve(pkt, wire_size); } -static int answer_prepare(struct kr_request *req, knot_pkt_t *query) -{ - knot_pkt_t *answer = req->answer; - if (knot_pkt_init_response(answer, query) != 0) { - return kr_error(ENOMEM); /* Failed to initialize answer */ - } - /* Handle EDNS in the query */ - if (knot_pkt_has_edns(query)) { - answer->opt_rr = knot_rrset_copy(req->ctx->downstream_opt_rr, &answer->mm); - if (answer->opt_rr == NULL){ - return kr_error(ENOMEM); - } - /* Set DO bit if set (DNSSEC requested). */ - if (knot_pkt_has_dnssec(query)) { - knot_edns_set_do(answer->opt_rr); - } - } - return kr_ok(); -} - /** * @param all_secure optionally &&-combine security of written RRs into its value. * (i.e. if you pass a pointer to false, it will always remain) @@ -726,11 +706,11 @@ static int query_finalize(struct kr_request *request, struct kr_query *qry, knot return kr_ok(); } -int kr_resolve_begin(struct kr_request *request, struct kr_context *ctx, knot_pkt_t *answer) +int kr_resolve_begin(struct kr_request *request, struct kr_context *ctx) { /* Initialize request */ request->ctx = ctx; - request->answer = answer; + request->answer = NULL; request->options = ctx->options; request->state = KR_STATE_CONSUME; request->current_query = NULL; @@ -781,20 +761,6 @@ static int resolve_query(struct kr_request *request, const knot_pkt_t *packet) } } - /* Initialize answer packet */ - knot_pkt_t *answer = request->answer; - knot_wire_set_qr(answer->wire); - knot_wire_clear_aa(answer->wire); - knot_wire_set_ra(answer->wire); - knot_wire_set_rcode(answer->wire, KNOT_RCODE_NOERROR); - - assert(request->qsource.packet); - if (knot_wire_get_cd(request->qsource.packet->wire)) { - knot_wire_set_cd(answer->wire); - } else if (qry->flags.DNSSEC_WANT) { - knot_wire_set_ad(answer->wire); - } - /* Expect answer, pop if satisfied immediately */ ITERATE_LAYERS(request, qry, begin); if ((request->state & KR_STATE_DONE) != 0) { @@ -808,6 +774,64 @@ static int resolve_query(struct kr_request *request, const knot_pkt_t *packet) return request->state; } +knot_pkt_t * kr_request_ensure_answer(struct kr_request *request) +{ + if (request->answer) + return request->answer; + + const knot_pkt_t *qs_pkt = request->qsource.packet; + assert(qs_pkt); + // Find answer_max: limit on DNS wire length. + size_t answer_max; + const struct kr_request_qsource_flags *qs_flags = &request->qsource.flags; + assert((qs_flags->tls || qs_flags->http) ? qs_flags->tcp : true); + if (!request->qsource.addr || qs_flags->tcp) { + // not on UDP + answer_max = KNOT_WIRE_MAX_PKTSIZE; + } else if (knot_pkt_has_edns(qs_pkt)) { + // UDP with EDNS + answer_max = MIN(knot_edns_get_payload(qs_pkt->opt_rr), + knot_edns_get_payload(request->ctx->downstream_opt_rr)); + answer_max = MAX(answer_max, KNOT_WIRE_MIN_PKTSIZE); + } else { + // UDP without EDNS + answer_max = KNOT_WIRE_MIN_PKTSIZE; + } + + // Allocate the packet. + knot_pkt_t *answer = request->answer = + knot_pkt_new(NULL, answer_max, &request->pool); + if (!answer || knot_pkt_init_response(answer, qs_pkt) != 0) { + assert(!answer); // otherwise we messed something up + goto enomem; + } + + uint8_t *wire = answer->wire; // much was done by knot_pkt_init_response() + knot_wire_set_ra(wire); + knot_wire_set_rcode(wire, KNOT_RCODE_NOERROR); + if (knot_wire_get_cd(qs_pkt->wire)) { + knot_wire_set_cd(wire); + } else if (request->current_query && request->current_query->flags.DNSSEC_WANT) { // FIXME: ugly + knot_wire_set_ad(wire); + } + + // Prepare EDNS if required. + if (knot_pkt_has_edns(qs_pkt)) { + answer->opt_rr = knot_rrset_copy(request->ctx->downstream_opt_rr, + &answer->mm); + if (!answer->opt_rr) + goto enomem; + if (knot_pkt_has_dnssec(qs_pkt)) + knot_edns_set_do(answer->opt_rr); + } + + return request->answer; +enomem: + // Consequences of `return NULL` would be way too complicated to handle. + kr_log_error("ERROR: irrecoverable out of memory condition in %s\n", __func__); + abort(); +} + KR_PURE static bool kr_inaddr_equal(const struct sockaddr *a, const struct sockaddr *b) { const int a_len = kr_inaddr_len(a); @@ -904,9 +928,6 @@ int kr_resolve_consume(struct kr_request *request, const struct sockaddr *src, k /* Empty resolution plan, push packet as the new query */ if (packet && kr_rplan_empty(rplan)) { - if (answer_prepare(request, packet) != 0) { - return KR_STATE_FAIL; - } return resolve_query(request, packet); } @@ -1633,6 +1654,7 @@ int kr_resolve_checkout(struct kr_request *request, const struct sockaddr *src, int kr_resolve_finish(struct kr_request *request, int state) { request->state = state; + kr_request_ensure_answer(request); /* Finalize answer and construct wire-buffer. */ ITERATE_LAYERS(request, NULL, answer_finalize); answer_finalize(request); diff --git a/lib/resolve.h b/lib/resolve.h index db596a387..6f5de2627 100644 --- a/lib/resolve.h +++ b/lib/resolve.h @@ -36,7 +36,7 @@ * }; * * // Setup and provide input query - * int state = kr_resolve_begin(&req, ctx, final_answer); + * int state = kr_resolve_begin(&req, ctx); * state = kr_resolve_consume(&req, query); * * // Generate answer @@ -179,7 +179,7 @@ struct kr_request_qsource_flags { */ struct kr_request { struct kr_context *ctx; - knot_pkt_t *answer; + knot_pkt_t *answer; /**< See kr_request_ensure_answer() */ struct kr_query *current_query; /**< Current evaluated query. */ struct { /** Address that originated the request. NULL for internal origin. */ @@ -234,16 +234,24 @@ struct kr_request { /** * Begin name resolution. * - * @note Expects a request to have an initialized mempool, the "answer" packet will - * be kept during the resolution and will contain the final answer at the end. + * @note Expects a request to have an initialized mempool. * * @param request request state with initialized mempool * @param ctx resolution context - * @param answer allocated packet for final answer * @return CONSUME (expecting query) */ KR_EXPORT -int kr_resolve_begin(struct kr_request *request, struct kr_context *ctx, knot_pkt_t *answer); +int kr_resolve_begin(struct kr_request *request, struct kr_context *ctx); + +/** + * Ensure that request->answer is usable, and return it (for convenience). + * + * It can not fail; FIXME: is it worth in the API to abort() instead of return NULL? + * Only use this when it's guaranteed that there will be no delay before sending it. + * You don't need to call this in places where "resolver knows" that there will be no delay. + */ +KR_EXPORT __attribute__((returns_nonnull)) +knot_pkt_t * kr_request_ensure_answer(struct kr_request *request); /** * Consume input packet (may be either first query or answer to query originated from kr_resolve_produce()) diff --git a/modules/cookies/cookiemonster.c b/modules/cookies/cookiemonster.c index 753785d7e..fa1b869eb 100644 --- a/modules/cookies/cookiemonster.c +++ b/modules/cookies/cookiemonster.c @@ -343,7 +343,7 @@ int check_request(kr_layer_t *ctx) return ctx->state; } - knot_pkt_t *answer = req->answer; + knot_pkt_t *answer = req->answer; // FIXME: see kr_request_ensure_answer() if (ctx->state & (KR_STATE_DONE | KR_STATE_FAIL)) { return ctx->state; diff --git a/modules/http/http_doh.test.lua b/modules/http/http_doh.test.lua index cfe31d373..194d08a8e 100644 --- a/modules/http/http_doh.test.lua +++ b/modules/http/http_doh.test.lua @@ -3,7 +3,7 @@ local basexx = require('basexx') local ffi = require('ffi') local function gen_huge_answer(_, req) - local answer = req.answer + local answer = req:ensure_answer() ffi.C.kr_pkt_make_auth_header(answer) answer:rcode(kres.rcode.NOERROR) @@ -19,7 +19,7 @@ end local function gen_varying_ttls(_, req) local qry = req:current() - local answer = req.answer + local answer = req:ensure_answer() ffi.C.kr_pkt_make_auth_header(answer) answer:rcode(kres.rcode.NOERROR) diff --git a/modules/policy/README.rst b/modules/policy/README.rst index cc346783e..aabf7a3f5 100644 --- a/modules/policy/README.rst +++ b/modules/policy/README.rst @@ -266,7 +266,7 @@ Custom actions -- Custom action which generates fake A record local ffi = require('ffi') local function fake_A_record(state, req) - local answer = req.answer + local answer = req:ensure_answer() local qry = req:current() if qry.stype ~= kres.type.A then return state diff --git a/modules/policy/policy.lua b/modules/policy/policy.lua index 070b2c08b..0617cce53 100644 --- a/modules/policy/policy.lua +++ b/modules/policy/policy.lua @@ -213,7 +213,7 @@ end function policy.ANSWER(rtable, nodata) return function(_, req) local qry = req:current() - local answer = req.answer + local answer = req:ensure_answer() local data = rtable[qry.stype] ffi.C.kr_pkt_make_auth_header(answer) @@ -254,7 +254,7 @@ local dname_localhost = todname('localhost.') -- Rule for localhost. zone; see RFC6303, sec. 3 local function localhost(_, req) local qry = req:current() - local answer = req.answer + local answer = req:ensure_answer() ffi.C.kr_pkt_make_auth_header(answer) local is_exact = ffi.C.knot_dname_is_equal(qry.sname, dname_localhost) @@ -286,7 +286,7 @@ local dname_rev4_localhost_apex = todname('127.in-addr.arpa'); -- TODO: much of this would better be left to the hints module (or coordinated). local function localhost_reversed(_, req) local qry = req:current() - local answer = req.answer + local answer = req:ensure_answer() -- classify qry.sname: local is_exact -- exact dname for localhost @@ -606,7 +606,7 @@ local function answer_clear(req) req.add_selected.len = 0 -- Let's be defensive and clear the answer, too. - local pkt = req.answer + local pkt = req:ensure_answer() pkt:clear_payload() return pkt end @@ -718,8 +718,8 @@ function policy.REFUSE(_, req) end function policy.TC(state, req) - -- Skip non-UDP queries - if req.answer.max_size == 65535 then + -- Avoid non-UDP queries + if req.qsource.flags.tcp then return state end diff --git a/modules/policy/policy.slice.test.lua b/modules/policy/policy.slice.test.lua index 5ee80cb06..9b83741f0 100644 --- a/modules/policy/policy.slice.test.lua +++ b/modules/policy/policy.slice.test.lua @@ -39,7 +39,7 @@ local function sliceaction(index) slice_queries[index][name] = count + 1 -- refuse query - local answer = req.answer + local answer = req:ensure_answer() answer:rcode(kres.rcode.REFUSED) answer:ad(false) return kres.DONE diff --git a/modules/rebinding/rebinding.lua b/modules/rebinding/rebinding.lua index d8b9f6627..99c2aa3c5 100644 --- a/modules/rebinding/rebinding.lua +++ b/modules/rebinding/rebinding.lua @@ -71,7 +71,7 @@ end local function refuse(req) policy.REFUSE(nil, req) - local pkt = req.answer + local pkt = req:ensure_answer() pkt:aa(false) pkt:begin(kres.section.ADDITIONAL) diff --git a/modules/refuse_nord/refuse_nord.c b/modules/refuse_nord/refuse_nord.c index 0a3d911a2..c9d55f4e9 100644 --- a/modules/refuse_nord/refuse_nord.c +++ b/modules/refuse_nord/refuse_nord.c @@ -15,7 +15,7 @@ static int refuse_nord_query(kr_layer_t *ctx) uint8_t rd = knot_wire_get_rd(req->qsource.packet->wire); if (!rd) { - knot_pkt_t *answer = req->answer; + knot_pkt_t *answer = kr_request_ensure_answer(req); knot_wire_set_rcode(answer->wire, KNOT_RCODE_REFUSED); knot_wire_clear_ad(answer->wire); ctx->state = KR_STATE_DONE; diff --git a/modules/ta_update/ta_update.test.lua b/modules/ta_update/ta_update.test.lua index 1828c4921..2263ac63e 100644 --- a/modules/ta_update/ta_update.test.lua +++ b/modules/ta_update/ta_update.test.lua @@ -11,7 +11,7 @@ trust_anchors.remove('.') -- count . IN DNSKEY queries counter = 0 local function counter_func (state, req) - local answer = req.answer + local answer = req:ensure_answer() local qry = req:current() if answer:qclass() == kres.class.IN and qry.stype == kres.type.DNSKEY diff --git a/modules/ta_update/ta_update.unmanagedkey.test.integr/kresd_config.j2 b/modules/ta_update/ta_update.unmanagedkey.test.integr/kresd_config.j2 index d0d72611b..95d1196a0 100644 --- a/modules/ta_update/ta_update.unmanagedkey.test.integr/kresd_config.j2 +++ b/modules/ta_update/ta_update.unmanagedkey.test.integr/kresd_config.j2 @@ -12,7 +12,6 @@ function inc_cb() end function check_status(state, query) - local answer = query.request.answer -- status was present for 10 days if cb_counter > 0 then return policy.DENY_MSG('check last answer') diff --git a/tests/config/doh2.test.lua b/tests/config/doh2.test.lua index 79b051df2..0d6a3eb61 100644 --- a/tests/config/doh2.test.lua +++ b/tests/config/doh2.test.lua @@ -3,7 +3,7 @@ local basexx = require('basexx') local ffi = require('ffi') local function gen_huge_answer(_, req) - local answer = req.answer + local answer = req:ensure_answer() ffi.C.kr_pkt_make_auth_header(answer) answer:rcode(kres.rcode.NOERROR) @@ -19,7 +19,7 @@ end local function gen_varying_ttls(_, req) local qry = req:current() - local answer = req.answer + local answer = req:ensure_answer() ffi.C.kr_pkt_make_auth_header(answer) answer:rcode(kres.rcode.NOERROR)