From 64c9c8ef3985bee02213e0726711e7893f5ce71f Mon Sep 17 00:00:00 2001 From: Aurelien DARRAGON Date: Mon, 11 Dec 2023 17:31:31 +0100 Subject: [PATCH] BUG/MINOR: server/dns: use server_set_inetaddr() to unset srv addr from DNS As seen before, server's addr and svc_port should not be updated directly during runtime, because even if the update is performed under the lock, some competing threads might be reading ->addr and ->svc_port without the lock because they simply cannot afford it. To prevent races with such competing threads, server's addr and port should only be updated using server_set_inetaddr() function or similar. This patch depends on: - "MINOR: server: ensure connection cleanup on server addr changes" - "CLEANUP: server/event_hdl: remove purge_conn hint in INETADDR event" - "MEDIUM: server: merge srv_update_addr() and srv_update_addr_port() logic" - "MEDIUM: server: make server_set_inetaddr() updater serializable" - "MINOR: server/event_hdl: expose updater info through INETADDR event" - "MINOR: server: add dns hint in server_inetaddr_updater struct" - "MEDIUM: server/dns: clear RMAINT when addr resolves again" While it could be backported in 2.9 with cd994407a ("BUG/MAJOR: server/addr: fix a race during server addr:svc_port updates") to ensure addr and svc_port reset performed by resolver's code comply with the API taking care of pushing the update (and thus avoid any race), some patch dependencies are quite sensitive so it's probably best to avoid backporting for no good reason, or at least wait for it to be considered stable to prevent any breakeages. --- src/resolvers.c | 7 +++++-- src/server.c | 24 +++++++++++++++++++----- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/resolvers.c b/src/resolvers.c index e02c8a561..ab9541467 100644 --- a/src/resolvers.c +++ b/src/resolvers.c @@ -659,14 +659,17 @@ static void leave_resolver_code() */ static void resolv_srvrq_cleanup_srv(struct server *srv) { + struct server_inetaddr srv_addr; + _resolv_unlink_resolution(srv->resolv_requester); HA_SPIN_LOCK(SERVER_LOCK, &srv->lock); srvrq_set_srv_down(srv); ha_free(&srv->hostname); ha_free(&srv->hostname_dn); srv->hostname_dn_len = 0; - memset(&srv->addr, 0, sizeof(srv->addr)); - srv->svc_port = 0; + memset(&srv_addr, 0, sizeof(srv_addr)); + /* unset server's addr */ + server_set_inetaddr(srv, &srv_addr, SERVER_INETADDR_UPDATER_NONE, NULL); srv->flags |= SRV_F_NO_RESOLUTION; ebpt_delete(&srv->host_dn); diff --git a/src/server.c b/src/server.c index c3ddcb2a6..8718adc91 100644 --- a/src/server.c +++ b/src/server.c @@ -4448,8 +4448,13 @@ int snr_resolution_cb(struct resolv_requester *requester, struct dns_counters *c srv_update_addr(s, firstip, firstip_sin_family, SERVER_INETADDR_UPDATER_DNS_CACHE); update_status: - if (!snr_set_srv_down(s, has_no_ip) && has_no_ip) - memset(&s->addr, 0, sizeof(s->addr)); + if (!snr_set_srv_down(s, has_no_ip) && has_no_ip) { + struct server_inetaddr s_addr; + + memset(&s_addr, 0, sizeof(s_addr)); + /* unset server's addr */ + server_set_inetaddr(s, &s_addr, SERVER_INETADDR_UPDATER_NONE, NULL); + } return 1; invalid: @@ -4457,8 +4462,13 @@ int snr_resolution_cb(struct resolv_requester *requester, struct dns_counters *c counters->app.resolver.invalid++; goto update_status; } - if (!snr_set_srv_down(s, has_no_ip) && has_no_ip) - memset(&s->addr, 0, sizeof(s->addr)); + if (!snr_set_srv_down(s, has_no_ip) && has_no_ip) { + struct server_inetaddr s_addr; + + memset(&s_addr, 0, sizeof(s_addr)); + /* unset server's addr */ + server_set_inetaddr(s, &s_addr, SERVER_INETADDR_UPDATER_NONE, NULL); + } return 0; } @@ -4539,7 +4549,11 @@ int snr_resolution_error_cb(struct resolv_requester *requester, int error_code) HA_SPIN_LOCK(SERVER_LOCK, &s->lock); if (!snr_set_srv_down(s, 1)) { - memset(&s->addr, 0, sizeof(s->addr)); + struct server_inetaddr s_addr; + + memset(&s_addr, 0, sizeof(s_addr)); + /* unset server's addr */ + server_set_inetaddr(s, &s_addr, SERVER_INETADDR_UPDATER_NONE, NULL); HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock); resolv_detach_from_resolution_answer_items(requester->resolution, requester); return 0;