From a458c2bfb01908d0ea495ce3f3340c9445402e09 Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Wed, 23 Apr 2025 12:14:00 +0100 Subject: [PATCH] Tidy up pipe-to-parent code in DNS TCP path. --- src/cache.c | 281 ++++++++++++++++++++++++++------------------------ src/dnsmasq.c | 11 +- src/dnsmasq.h | 7 ++ 3 files changed, 164 insertions(+), 135 deletions(-) diff --git a/src/cache.c b/src/cache.c index 5cae80c..9600a3c 100644 --- a/src/cache.c +++ b/src/cache.c @@ -799,14 +799,16 @@ void cache_end_insert(void) char *name = cache_get_name(new_chain); ssize_t m = strlen(name); unsigned int flags = new_chain->flags; + unsigned char op = PIPE_OP_RR; #ifdef HAVE_DNSSEC u16 class = new_chain->uid; #endif + read_write(daemon->pipe_to_parent, &op, sizeof(op), RW_WRITE); read_write(daemon->pipe_to_parent, (unsigned char *)&m, sizeof(m), RW_WRITE); read_write(daemon->pipe_to_parent, (unsigned char *)name, m, RW_WRITE); read_write(daemon->pipe_to_parent, (unsigned char *)&new_chain->ttd, sizeof(new_chain->ttd), RW_WRITE); - read_write(daemon->pipe_to_parent, (unsigned char *)&flags, sizeof(flags), RW_WRITE); + read_write(daemon->pipe_to_parent, (unsigned char *)&flags, sizeof(flags), RW_WRITE); read_write(daemon->pipe_to_parent, (unsigned char *)&new_chain->addr, sizeof(new_chain->addr), RW_WRITE); if (flags & F_RR) @@ -838,27 +840,29 @@ void cache_end_insert(void) /* signal end of cache insert in master process */ if (daemon->pipe_to_parent != -1) { - ssize_t m = -1; - - read_write(daemon->pipe_to_parent, (unsigned char *)&m, sizeof(m), RW_WRITE); - -#ifdef HAVE_DNSSEC - /* Sneak out possibly updated crypto HWM values. */ - read_write(daemon->pipe_to_parent, - (unsigned char *)&daemon->metrics[METRIC_CRYPTO_HWM], - sizeof(daemon->metrics[METRIC_CRYPTO_HWM]), RW_WRITE); - read_write(daemon->pipe_to_parent, - (unsigned char *)&daemon->metrics[METRIC_SIG_FAIL_HWM], - sizeof(daemon->metrics[METRIC_SIG_FAIL_HWM]), RW_WRITE); - read_write(daemon->pipe_to_parent, - (unsigned char *)&daemon->metrics[METRIC_WORK_HWM], - sizeof(daemon->metrics[METRIC_WORK_HWM]), RW_WRITE); -#endif + unsigned char op = PIPE_OP_END; + read_write(daemon->pipe_to_parent, &op, sizeof(op), RW_WRITE); } - - new_chain = NULL; } +#ifdef HAVE_DNSSEC +void cache_update_hwm(void) +{ + /* Sneak out possibly updated crypto HWM values. */ + unsigned char op = PIPE_OP_STATS; + + read_write(daemon->pipe_to_parent, &op, sizeof(op), RW_WRITE); + read_write(daemon->pipe_to_parent, + (unsigned char *)&daemon->metrics[METRIC_CRYPTO_HWM], + sizeof(daemon->metrics[METRIC_CRYPTO_HWM]), RW_WRITE); + read_write(daemon->pipe_to_parent, + (unsigned char *)&daemon->metrics[METRIC_SIG_FAIL_HWM], + sizeof(daemon->metrics[METRIC_SIG_FAIL_HWM]), RW_WRITE); + read_write(daemon->pipe_to_parent, + (unsigned char *)&daemon->metrics[METRIC_WORK_HWM], + sizeof(daemon->metrics[METRIC_WORK_HWM]), RW_WRITE); +} +#endif /* A marshalled cache entry arrives on fd, read, unmarshall and insert into cache of master process. */ int cache_recv_insert(time_t now, int fd) @@ -869,139 +873,150 @@ int cache_recv_insert(time_t now, int fd) time_t ttd; unsigned int flags; struct crec *crecp = NULL; + unsigned char op; cache_start_insert(); while (1) { - - if (!read_write(fd, (unsigned char *)&m, sizeof(m), RW_READ)) + if (!read_write(fd, &op, sizeof(op), RW_READ)) return 0; - if (m == -1) + switch (op) { -#ifdef HAVE_DNSSEC - /* Sneak in possibly updated crypto HWM. */ - unsigned int val; - - if (!read_write(fd, (unsigned char *)&val, sizeof(val), RW_READ)) - return 0; - if (val > daemon->metrics[METRIC_CRYPTO_HWM]) - daemon->metrics[METRIC_CRYPTO_HWM] = val; - if (!read_write(fd, (unsigned char *)&val, sizeof(val), RW_READ)) - return 0; - if (val > daemon->metrics[METRIC_SIG_FAIL_HWM]) - daemon->metrics[METRIC_SIG_FAIL_HWM] = val; - if (!read_write(fd, (unsigned char *)&val, sizeof(val), RW_READ)) - return 0; - if (val > daemon->metrics[METRIC_WORK_HWM]) - daemon->metrics[METRIC_WORK_HWM] = val; -#endif + case PIPE_OP_END: cache_end_insert(); return 1; - } - + #ifdef HAVE_DNSSEC - /* UDP validation moved to TCP to avoid truncation. - Restart UDP validation process with the returned result. */ - if (m == -2) - { - int status, uid, keycount, validatecount; - int *keycountp, *validatecountp; - size_t ret_len; + case PIPE_OP_STATS: + { + /* Sneak in possibly updated crypto HWM. */ + unsigned int val; + + if (!read_write(fd, (unsigned char *)&val, sizeof(val), RW_READ)) + return 0; + if (val > daemon->metrics[METRIC_CRYPTO_HWM]) + daemon->metrics[METRIC_CRYPTO_HWM] = val; + if (!read_write(fd, (unsigned char *)&val, sizeof(val), RW_READ)) + return 0; + if (val > daemon->metrics[METRIC_SIG_FAIL_HWM]) + daemon->metrics[METRIC_SIG_FAIL_HWM] = val; + if (!read_write(fd, (unsigned char *)&val, sizeof(val), RW_READ)) + return 0; + if (val > daemon->metrics[METRIC_WORK_HWM]) + daemon->metrics[METRIC_WORK_HWM] = val; + return 1; + } - struct frec *forward; - - if (!read_write(fd, (unsigned char *)&status, sizeof(status), RW_READ)) - return 0; - if (!read_write(fd, (unsigned char *)&ret_len, sizeof(ret_len), RW_READ)) - return 0; - if (!read_write(fd, (unsigned char *)daemon->packet, ret_len, RW_READ)) - return 0; - if (!read_write(fd, (unsigned char *)&forward, sizeof(forward), RW_READ)) - return 0; - if (!read_write(fd, (unsigned char *)&uid, sizeof(uid), RW_READ)) - return 0; - if (!read_write(fd, (unsigned char *)&keycount, sizeof(keycount), RW_READ)) - return 0; - if (!read_write(fd, (unsigned char *)&keycountp, sizeof(keycountp), RW_READ)) - return 0; - if (!read_write(fd, (unsigned char *)&validatecount, sizeof(validatecount), RW_READ)) - return 0; - if (!read_write(fd, (unsigned char *)&validatecountp, sizeof(validatecountp), RW_READ)) - return 0; - - /* There's a tiny chance that the frec may have been freed - and reused before the TCP process returns. Detect that with - the uid field which is unique modulo 2^32 for each use. */ - if (uid == forward->uid) - { - /* repatriate the work counters from the child process. */ - *keycountp = keycount; - *validatecountp = validatecount; - - if (!forward->dependent) - return_reply(now, forward, (struct dns_header *)daemon->packet, ret_len, status); - else - pop_and_retry_query(forward, status, now); - } - - return 1; - } + case PIPE_OP_RESULT: + { + /* UDP validation moved to TCP to avoid truncation. + Restart UDP validation process with the returned result. */ + int status, uid, keycount, validatecount; + int *keycountp, *validatecountp; + size_t ret_len; + + struct frec *forward; + + if (!read_write(fd, (unsigned char *)&status, sizeof(status), RW_READ)) + return 0; + if (!read_write(fd, (unsigned char *)&ret_len, sizeof(ret_len), RW_READ)) + return 0; + if (!read_write(fd, (unsigned char *)daemon->packet, ret_len, RW_READ)) + return 0; + if (!read_write(fd, (unsigned char *)&forward, sizeof(forward), RW_READ)) + return 0; + if (!read_write(fd, (unsigned char *)&uid, sizeof(uid), RW_READ)) + return 0; + if (!read_write(fd, (unsigned char *)&keycount, sizeof(keycount), RW_READ)) + return 0; + if (!read_write(fd, (unsigned char *)&keycountp, sizeof(keycountp), RW_READ)) + return 0; + if (!read_write(fd, (unsigned char *)&validatecount, sizeof(validatecount), RW_READ)) + return 0; + if (!read_write(fd, (unsigned char *)&validatecountp, sizeof(validatecountp), RW_READ)) + return 0; + + /* There's a tiny chance that the frec may have been freed + and reused before the TCP process returns. Detect that with + the uid field which is unique modulo 2^32 for each use. */ + if (uid == forward->uid) + { + /* repatriate the work counters from the child process. */ + *keycountp = keycount; + *validatecountp = validatecount; + + if (!forward->dependent) + return_reply(now, forward, (struct dns_header *)daemon->packet, ret_len, status); + else + pop_and_retry_query(forward, status, now); + } + + return 1; + } #endif - - if (!read_write(fd, (unsigned char *)daemon->namebuff, m, RW_READ) || - !read_write(fd, (unsigned char *)&ttd, sizeof(ttd), RW_READ) || - !read_write(fd, (unsigned char *)&flags, sizeof(flags), RW_READ) || - !read_write(fd, (unsigned char *)&addr, sizeof(addr), RW_READ)) - return 0; - - daemon->namebuff[m] = 0; - - ttl = difftime(ttd, now); - - if (flags & F_CNAME) - { - struct crec *newc = really_insert(daemon->namebuff, NULL, C_IN, now, ttl, flags); - /* This relies on the fact that the target of a CNAME immediately precedes - it because of the order of extraction in extract_addresses, and - the order reversal on the new_chain. */ - if (newc) + + case PIPE_OP_RR: + if (!read_write(fd, (unsigned char *)&m, sizeof(m), RW_READ) || + !read_write(fd, (unsigned char *)daemon->namebuff, m, RW_READ) || + !read_write(fd, (unsigned char *)&ttd, sizeof(ttd), RW_READ) || + !read_write(fd, (unsigned char *)&flags, sizeof(flags), RW_READ) || + !read_write(fd, (unsigned char *)&addr, sizeof(addr), RW_READ)) + return 0; + + daemon->namebuff[m] = 0; + + ttl = difftime(ttd, now); + + if (flags & F_CNAME) { - newc->addr.cname.is_name_ptr = 0; - - if (!crecp) - newc->addr.cname.target.cache = NULL; - else + struct crec *newc = really_insert(daemon->namebuff, NULL, C_IN, now, ttl, flags); + /* This relies on the fact that the target of a CNAME immediately precedes + it because of the order of extraction in extract_addresses, and + the order reversal on the new_chain. */ + if (newc) { - next_uid(crecp); - newc->addr.cname.target.cache = crecp; - newc->addr.cname.uid = crecp->uid; + newc->addr.cname.is_name_ptr = 0; + + if (!crecp) + newc->addr.cname.target.cache = NULL; + else + { + next_uid(crecp); + newc->addr.cname.target.cache = crecp; + newc->addr.cname.uid = crecp->uid; + } } } - } - else - { - unsigned short class = C_IN; - - if ((flags & F_RR) && !(flags & F_NEG) && (flags & F_KEYTAG) - && !(addr.rrblock.rrdata = blockdata_read(fd, addr.rrblock.datalen))) - return 0; + else + { + unsigned short class = C_IN; + + if ((flags & F_RR) && !(flags & F_NEG) && (flags & F_KEYTAG) + && !(addr.rrblock.rrdata = blockdata_read(fd, addr.rrblock.datalen))) + return 0; #ifdef HAVE_DNSSEC - if (flags & F_DNSKEY) - { - if (!read_write(fd, (unsigned char *)&class, sizeof(class), RW_READ) || - !(addr.key.keydata = blockdata_read(fd, addr.key.keylen))) - return 0; - } - else if (flags & F_DS) - { - if (!read_write(fd, (unsigned char *)&class, sizeof(class), RW_READ) || - (!(flags & F_NEG) && !(addr.key.keydata = blockdata_read(fd, addr.key.keylen)))) - return 0; - } + if (flags & F_DNSKEY) + { + if (!read_write(fd, (unsigned char *)&class, sizeof(class), RW_READ) || + !(addr.key.keydata = blockdata_read(fd, addr.key.keylen))) + return 0; + } + else if (flags & F_DS) + { + if (!read_write(fd, (unsigned char *)&class, sizeof(class), RW_READ) || + (!(flags & F_NEG) && !(addr.key.keydata = blockdata_read(fd, addr.key.keylen)))) + return 0; + } #endif - crecp = really_insert(daemon->namebuff, &addr, class, now, ttl, flags); + crecp = really_insert(daemon->namebuff, &addr, class, now, ttl, flags); + } + + /* loop reading RRs, since we don't want to go back to the poll() loop + and start processing other queries which might pollute the insertion + chain. The child will never block between the first OP_RR and the OP_END */ + continue; } } } diff --git a/src/dnsmasq.c b/src/dnsmasq.c index 944b4cc..1759b8e 100644 --- a/src/dnsmasq.c +++ b/src/dnsmasq.c @@ -2124,6 +2124,10 @@ static void do_tcp_connection(struct listener *listener, time_t now, int slot) if (!option_bool(OPT_DEBUG)) { +#ifdef HAVE_DNSSEC + cache_update_hwm(); /* Sneak out possibly updated crypto HWM values. */ +#endif + close(daemon->pipe_to_parent); flush_log(); _exit(0); @@ -2229,10 +2233,10 @@ int swap_to_tcp(struct frec *forward, time_t now, int status, struct dns_header if (!option_bool(OPT_DEBUG)) { - ssize_t m = -2; + unsigned char op = PIPE_OP_RESULT; /* tell our parent we're done, and what the result was then exit. */ - read_write(daemon->pipe_to_parent, (unsigned char *)&m, sizeof(m), RW_WRITE); + read_write(daemon->pipe_to_parent, &op, sizeof(op), RW_WRITE); read_write(daemon->pipe_to_parent, (unsigned char *)&status, sizeof(status), RW_WRITE); read_write(daemon->pipe_to_parent, (unsigned char *)plen, sizeof(*plen), RW_WRITE); read_write(daemon->pipe_to_parent, (unsigned char *)header, *plen, RW_WRITE); @@ -2242,6 +2246,9 @@ int swap_to_tcp(struct frec *forward, time_t now, int status, struct dns_header read_write(daemon->pipe_to_parent, (unsigned char *)&keycount, sizeof(keycount), RW_WRITE); read_write(daemon->pipe_to_parent, (unsigned char *)validatecount, sizeof(*validatecount), RW_WRITE); read_write(daemon->pipe_to_parent, (unsigned char *)&validatecount, sizeof(validatecount), RW_WRITE); + + cache_update_hwm(); /* Sneak out possibly updated crypto HWM values. */ + close(daemon->pipe_to_parent); flush_log(); diff --git a/src/dnsmasq.h b/src/dnsmasq.h index e4af90d..c310597 100644 --- a/src/dnsmasq.h +++ b/src/dnsmasq.h @@ -537,6 +537,10 @@ struct crec { #define SRC_HOSTS 2 #define SRC_AH 3 +#define PIPE_OP_RR 1 /* Resource record */ +#define PIPE_OP_END 2 /* Cache entry complete: commit */ +#define PIPE_OP_RESULT 3 /* validation result. */ +#define PIPE_OP_STATS 4 /* Update parent's stats */ /* struct sockaddr is not large enough to hold any address, and specifically not big enough to hold an IPv6 address. @@ -1354,6 +1358,9 @@ void cache_end_insert(void); void cache_start_insert(void); unsigned int cache_remove_uid(const unsigned int uid); int cache_recv_insert(time_t now, int fd); +#ifdef HAVE_DNSSEC +void cache_update_hwm(void); +#endif struct crec *cache_insert(char *name, union all_addr *addr, unsigned short class, time_t now, unsigned long ttl, unsigned int flags); void cache_reload(void);