On Fri, Dec 15, 2023 at 02:54:19PM +0100, Otto Moerbeek wrote: > On Sun, Dec 10, 2023 at 09:57:08AM +0100, Otto Moerbeek wrote: > > > On Fri, Dec 01, 2023 at 09:18:32PM +0000, guilherme.janc...@yandex.com > > wrote: > > > > > >Synopsis: Repeated NTP peers in OpenNTPD > > > >Category: user > > > >Environment: > > > System : OpenBSD 7.4 > > > Details : OpenBSD 7.4 (GENERIC.MP) #0: Sun Oct 22 12:13:42 MDT 2023 > > > > > > r...@syspatch-74-amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP > > > > > > Architecture: OpenBSD.amd64 > > > Machine : amd64 > > > >Description: > > > If the same address/domain is specified multiple times in > > > OpenNTPD's configuration file, or if multiple domains resolve > > > to the same IP address, OpenNTPD will treat the same IP address > > > as if it was multiple peers. > > > >How-To-Repeat: > > > This can be tested by appending `server 127.0.0.1` multiple > > > times to the configuration file. > > > > > > Alternatively, assuming a default OpenNTPD configuration file > > > from OpenBSD 7.4, the following entries can be added to > > > /etc/hosts: > > > 127.0.0.1 time.cloudflare.com > > > 127.0.0.1 pool.ntp.org > > > > > > I noticed this bug using the default 7.4 configuration file. It > > > can happen because time.cloudflare.com is part of pool.ntp.org: > > > https://www.ntppool.org/scores/162.159.200.1 > > > https://www.ntppool.org/scores/162.159.200.123 > > > >Fix: > > > Removing the `server time.cloudflare.com` line from the > > > configuration file is a simple fix the user can make, but > > > OpenNTPD should check if an IP address it tries to add to the > > > list of peers is already a peer, and ignore it if so. If a > > > server is added with the `server` (not `servers`) keyword in the > > > configuration file, OpenNTPD should try the next IP the domain > > > resolves to if applicable. > > > > > > > Thanks for the report, I'll take a look. > > > > -Otto > > > > Due to verious reasons this is all a bit complicated, I did not find a > nice solution yet. Some patience required. > > -Otto >
Found some more time. Try this. The approach is: we assume the addresses of a pool (from the "servers" keyword) vary over time. So if one of the pool addresses is in the address list of a peer ("server") we skip it ad try to re-resolve the pool, lookin for a new address, as we already did earlier when a pool member doe snot respond. I decided to not implement "the move to abother address of the list" in the "server" case. The address logic is already complex enough. -Otto Index: client.c =================================================================== RCS file: /home/cvs/src/usr.sbin/ntpd/client.c,v diff -u -p -r1.117 client.c --- client.c 24 Mar 2022 07:37:19 -0000 1.117 +++ client.c 17 Dec 2023 09:13:16 -0000 @@ -108,6 +108,7 @@ client_nextaddr(struct ntp_peer *p) return (-1); if (p->addr_head.a == NULL) { + log_debug("kicking query for %s", p->addr_head.name); priv_dns(IMSG_HOST_DNS, p->addr_head.name, p->id); p->state = STATE_DNS_INPROGRESS; return (-1); @@ -145,7 +146,15 @@ client_query(struct ntp_peer *p) } if (p->state < STATE_DNS_DONE || p->addr == NULL) - return (-1); + return (0); + + /* if we're a pool member and a peer has taken our address, signal */ + if (p->addr_head.pool != 0 && addr_already_used(&p->addr->ss)) { + log_debug("pool addr %s used by peer, will reresolve pool", + log_sockaddr((struct sockaddr *)&p->addr->ss)); + p->senderrors++; + return (0); + } if (p->query.fd == -1) { struct sockaddr *sa = (struct sockaddr *)&p->addr->ss; Index: ntp.c =================================================================== RCS file: /home/cvs/src/usr.sbin/ntpd/ntp.c,v diff -u -p -r1.171 ntp.c --- ntp.c 6 Dec 2023 15:51:53 -0000 1.171 +++ ntp.c 17 Dec 2023 09:13:16 -0000 @@ -54,8 +54,6 @@ int ntp_dispatch_imsg(void); int ntp_dispatch_imsg_dns(void); void peer_add(struct ntp_peer *); void peer_remove(struct ntp_peer *); -int inpool(struct sockaddr_storage *, - struct sockaddr_storage[MAX_SERVERS_DNS], size_t); void ntp_sighdlr(int sig) @@ -524,23 +522,52 @@ ntp_dispatch_imsg(void) return (0); } -int +static int +addr_equal(struct sockaddr_storage *a, struct sockaddr_storage *b) +{ + if (a->ss_family != b->ss_family) + return 0; + if (a->ss_family == AF_INET) { + if (((struct sockaddr_in *)a)->sin_addr.s_addr == + ((struct sockaddr_in *)b)->sin_addr.s_addr) + return 1; + } else if (memcmp(&((struct sockaddr_in6 *)a)->sin6_addr, + &((struct sockaddr_in6 *)b)->sin6_addr, + sizeof(struct sockaddr_in6)) == 0) { + return 1; + } + return 0; +} + +static int inpool(struct sockaddr_storage *a, struct sockaddr_storage old[MAX_SERVERS_DNS], size_t n) { size_t i; for (i = 0; i < n; i++) { - if (a->ss_family != old[i].ss_family) + if (addr_equal(a, &old[i])) + return 1; + } + return 0; +} + +/* check to see af an address is already listed by a "server" peer + in that case, it does not get added to a pool */ +int +addr_already_used(struct sockaddr_storage *a) +{ + struct ntp_peer *peer; + struct ntp_addr *addr; + + TAILQ_FOREACH(peer, &conf->ntp_peers, entry) { + if (peer->addr_head.pool != 0) continue; - if (a->ss_family == AF_INET) { - if (((struct sockaddr_in *)a)->sin_addr.s_addr == - ((struct sockaddr_in *)&old[i])->sin_addr.s_addr) + addr = peer->addr_head.a; + while (addr != NULL) { + if (addr_equal(a, &addr->ss)) return 1; - } else if (memcmp(&((struct sockaddr_in6 *)a)->sin6_addr, - &((struct sockaddr_in6 *)&old[i])->sin6_addr, - sizeof(struct sockaddr_in6)) == 0) { - return 1; + addr = addr->next; } } return 0; @@ -628,8 +655,11 @@ ntp_dispatch_imsg_dns(void) free(h); continue; } - if (inpool(&h->ss, existing, - n)) { + if (addr_already_used(&h->ss) || + inpool(&h->ss, existing, n)) { + log_debug("skipping address %s for %s", + log_sockaddr((struct sockaddr *) + &h->ss), peer->addr_head.name); free(h); continue; } @@ -660,8 +690,11 @@ ntp_dispatch_imsg_dns(void) } if (dlen != 0) fatalx("IMSG_HOST_DNS: dlen != 0"); - if (peer->addr_head.pool) - peer_remove(peer); + if (peer->addr_head.pool) { + if (peercount > addrcount) + peer_remove(peer); + peer->state = STATE_NONE; + } else client_addr_init(peer); break; Index: ntpd.h =================================================================== RCS file: /home/cvs/src/usr.sbin/ntpd/ntpd.h,v diff -u -p -r1.152 ntpd.h --- ntpd.h 27 Nov 2022 13:19:00 -0000 1.152 +++ ntpd.h 17 Dec 2023 09:13:16 -0000 @@ -371,6 +371,7 @@ int server_dispatch(int, struct ntpd_con int client_peer_init(struct ntp_peer *); int client_addr_init(struct ntp_peer *); int client_nextaddr(struct ntp_peer *); +int addr_already_used(struct sockaddr_storage *); int client_query(struct ntp_peer *); int client_dispatch(struct ntp_peer *, u_int8_t, u_int8_t); void client_log_error(struct ntp_peer *, const char *, int);