From f04514e98acdfb47939d2f447113587a2e31361e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 10 Sep 2024 14:18:48 +0200 Subject: [PATCH] Make dns_validator_cancel() respect the data ownership There was a data race dns_validator_cancel() was called when the offloaded operations were in progress. Make dns_validator_cancel() respect the data ownership and only set new .shuttingdown variable when the offloaded operations are in progress. The cancel operation would then finish when the offloaded work passes the ownership back to the respective thread. (cherry picked from commit ee122ba0251f7f5dcc0542647743b0c1408d674b) --- lib/dns/include/dns/validator.h | 1 + lib/dns/validator.c | 81 +++++++++++++++++++++++---------- 2 files changed, 57 insertions(+), 25 deletions(-) diff --git a/lib/dns/include/dns/validator.h b/lib/dns/include/dns/validator.h index 02058d115b..249687d756 100644 --- a/lib/dns/include/dns/validator.h +++ b/lib/dns/include/dns/validator.h @@ -122,6 +122,7 @@ struct dns_validator { bool secure; /* Internal validator state */ + atomic_bool canceling; unsigned int attributes; dns_fetch_t *fetch; dns_validator_t *subvalidator; diff --git a/lib/dns/validator.c b/lib/dns/validator.c index 8778a4df91..758a2cee74 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -77,6 +78,8 @@ enum valattr { VALATTR_INSECURITY = 1 << 4, /*%< Attempting proveunsecure. */ VALATTR_MAXVALIDATIONS = 1 << 5, /*%< Max validations quota */ VALATTR_MAXVALIDATIONFAILS = 1 << 6, /*%< Max validation fails quota */ + VALATTR_OFFLOADED = 1 << 7, /*%< The ownership has been passed to + offloaded thread */ /*! * NSEC proofs to be looked for. @@ -105,8 +108,10 @@ enum valattr { #define FOUNDCLOSEST(val) ((val->attributes & VALATTR_FOUNDCLOSEST) != 0) #define FOUNDOPTOUT(val) ((val->attributes & VALATTR_FOUNDOPTOUT) != 0) -#define CANCELED(v) (((v)->attributes & VALATTR_CANCELED) != 0) -#define COMPLETE(v) (((v)->attributes & VALATTR_COMPLETE) != 0) +#define CANCELING(v) atomic_load(&(v)->canceling) +#define CANCELED(v) (((v)->attributes & VALATTR_CANCELED) != 0) +#define OFFLOADED(v) (((v)->attributes & VALATTR_OFFLOADED) != 0) +#define COMPLETE(v) (((v)->attributes & VALATTR_COMPLETE) != 0) #define NEGATIVE(r) (((r)->attributes & DNS_RDATASETATTR_NEGATIVE) != 0) #define NXDOMAIN(r) (((r)->attributes & DNS_RDATASETATTR_NXDOMAIN) != 0) @@ -407,7 +412,7 @@ fetch_callback_dnskey(void *arg) { validator_log(val, ISC_LOG_DEBUG(3), "in fetch_callback_dnskey"); dns_resolver_destroyfetch(&val->fetch); - if (CANCELED(val)) { + if (CANCELED(val) || CANCELING(val)) { result = ISC_R_CANCELED; goto cleanup; } @@ -481,7 +486,7 @@ fetch_callback_ds(void *arg) { validator_log(val, ISC_LOG_DEBUG(3), "in fetch_callback_ds"); dns_resolver_destroyfetch(&val->fetch); - if (CANCELED(val)) { + if (CANCELED(val) || CANCELING(val)) { result = ISC_R_CANCELED; goto cleanup; } @@ -591,7 +596,7 @@ validator_callback_dnskey(void *arg) { val->subvalidator = NULL; - if (CANCELED(val)) { + if (CANCELED(val) || CANCELING(val)) { result = ISC_R_CANCELED; goto cleanup; } @@ -640,7 +645,7 @@ validator_callback_ds(void *arg) { val->subvalidator = NULL; - if (CANCELED(val)) { + if (CANCELED(val) || CANCELING(val)) { result = ISC_R_CANCELED; goto cleanup; } @@ -701,7 +706,7 @@ validator_callback_cname(void *arg) { val->subvalidator = NULL; - if (CANCELED(val)) { + if (CANCELED(val) || CANCELING(val)) { result = ISC_R_CANCELED; goto cleanup; } @@ -746,7 +751,7 @@ validator_callback_nsec(void *arg) { val->subvalidator = NULL; - if (CANCELED(val)) { + if (CANCELED(val) || CANCELING(val)) { result = ISC_R_CANCELED; goto cleanup; } @@ -1499,6 +1504,9 @@ validate_answer_process(void *arg); static void validate_answer_iter_done(dns_validator_t *val, isc_result_t result); +static void +validator_cancel_finish(dns_validator_t *validator); + static void validate_answer_iter_start(dns_validator_t *val) { isc_result_t result = ISC_R_SUCCESS; @@ -1507,7 +1515,9 @@ validate_answer_iter_start(dns_validator_t *val) { * Caller must be holding the validator lock. */ - if (CANCELED(val)) { + val->attributes &= ~VALATTR_OFFLOADED; + if (CANCELING(val)) { + validator_cancel_finish(val); result = ISC_R_CANCELED; goto cleanup; } @@ -1535,7 +1545,9 @@ validate_answer_iter_next(void *arg) { dns_validator_t *val = arg; isc_result_t result; - if (CANCELED(val)) { + val->attributes &= ~VALATTR_OFFLOADED; + if (CANCELING(val)) { + validator_cancel_finish(val); result = ISC_R_CANCELED; goto cleanup; } @@ -1563,7 +1575,7 @@ validate_answer_signing_key(void *arg) { dns_validator_t *val = arg; isc_result_t result = ISC_R_NOTFOUND; - if (CANCELED(val)) { + if (CANCELED(val) || CANCELING(val)) { val->result = ISC_R_CANCELED; } else { val->result = verify(val, val->key, &val->rdata, @@ -1600,7 +1612,9 @@ static void validate_answer_signing_key_done(void *arg) { dns_validator_t *val = arg; - if (CANCELED(val)) { + val->attributes &= ~VALATTR_OFFLOADED; + if (CANCELING(val)) { + validator_cancel_finish(val); val->result = ISC_R_CANCELED; } else if (val->key != NULL) { /* Process with next key if we selected one */ @@ -1616,7 +1630,9 @@ validate_answer_process(void *arg) { dns_validator_t *val = arg; isc_result_t result; - if (CANCELED(val)) { + val->attributes &= ~VALATTR_OFFLOADED; + if (CANCELING(val)) { + validator_cancel_finish(val); result = ISC_R_CANCELED; goto cleanup; } @@ -1770,6 +1786,7 @@ validate_answer_iter_done(dns_validator_t *val, isc_result_t result) { static void resume_answer(void *arg) { dns_validator_t *val = arg; + val->resume = true; validate_answer_iter_start(val); } @@ -1789,6 +1806,7 @@ validate_async_run(dns_validator_t *val, isc_job_cb cb) { static isc_result_t validate_helper_run(dns_validator_t *val, isc_job_cb cb) { + val->attributes |= VALATTR_OFFLOADED; isc_helper_run(val->loop, cb, val); return DNS_R_WAIT; } @@ -2045,7 +2063,7 @@ static void validate_dnskey_dsset_next(void *arg) { dns_validator_t *val = arg; - if (CANCELED(val)) { + if (CANCELED(val) || CANCELING(val)) { val->result = ISC_R_CANCELED; } else { val->result = dns_rdataset_next(val->dsset); @@ -2064,7 +2082,9 @@ validate_dnskey_dsset_next_done(void *arg) { dns_validator_t *val = arg; isc_result_t result = val->result; - if (CANCELED(val)) { + val->attributes &= ~VALATTR_OFFLOADED; + if (CANCELING(val)) { + validator_cancel_finish(val); result = ISC_R_CANCELED; } @@ -2091,7 +2111,7 @@ static void validate_dnskey_dsset_first(dns_validator_t *val) { isc_result_t result; - if (CANCELED(val)) { + if (CANCELED(val) || CANCELING(val)) { result = ISC_R_CANCELED; } else { result = dns_rdataset_first(val->dsset); @@ -2117,7 +2137,7 @@ validate_dnskey(void *arg) { dns_keynode_t *keynode = NULL; dns_rdata_ds_t ds; - if (CANCELED(val)) { + if (CANCELED(val) || CANCELING(val)) { result = ISC_R_CANCELED; goto cleanup; } @@ -3280,7 +3300,7 @@ validator_start(void *arg) { dns_validator_t *val = (dns_validator_t *)arg; isc_result_t result = ISC_R_FAILURE; - if (CANCELED(val)) { + if (CANCELED(val) || CANCELING(val)) { result = ISC_R_CANCELED; goto cleanup; } @@ -3447,14 +3467,11 @@ dns_validator_send(dns_validator_t *val) { (void)validate_async_run(val, validator_start); } -void -dns_validator_cancel(dns_validator_t *validator) { - REQUIRE(VALID_VALIDATOR(validator)); - REQUIRE(validator->tid == isc_tid()); +static void +validator_cancel_finish(dns_validator_t *validator) { + validator_log(validator, ISC_LOG_DEBUG(3), "validator_cancel_finish"); - validator_log(validator, ISC_LOG_DEBUG(3), "dns_validator_cancel"); - - if (!CANCELED(validator)) { + if (CANCELING(validator) && !CANCELED(validator)) { if (validator->fetch != NULL) { dns_resolver_cancelfetch(validator->fetch); } @@ -3469,6 +3486,20 @@ dns_validator_cancel(dns_validator_t *validator) { } } +void +dns_validator_cancel(dns_validator_t *validator) { + REQUIRE(VALID_VALIDATOR(validator)); + REQUIRE(validator->tid == isc_tid()); + + validator_log(validator, ISC_LOG_DEBUG(3), "dns_validator_cancel"); + + atomic_store(&validator->canceling, true); + + if (!OFFLOADED(validator)) { + validator_cancel_finish(validator); + } +} + static void destroy_validator(dns_validator_t *val) { isc_mem_t *mctx = NULL;