From b213205f423ae211533f3264bb9d81cd2e4c4c65 Mon Sep 17 00:00:00 2001 From: Aurelien DARRAGON Date: Thu, 7 Dec 2023 17:08:08 +0100 Subject: [PATCH] BUG/MINOR: server/event_hdl: propagate map port info through inetaddr event server addr:svc_port updates during runtime might set or clear the SRV_F_MAPPORTS flag. Unfortunately, the flag update is still directly performed by srv_update_addr_port() function while the addr:svc_port update is being scheduled for atomic update. Given that existing readers don't take server's lock to read addr:svc_port, they also check the SRV_F_MAPPORTS flag right after without the lock. So we could cause the readers to incorrectly interpret the svc_port from the server struct because the mapport information is not published atomically, resulting in inconsistencies between svc_port / mapport flag. (MAPPORTS flag causes svc_port to be used differently by the reader) To fix this, we publish the mapport information within the INETADDR server event and we let the task responsible for updating server's addr and port position or clear the flag depending on the mapport hint. This patch depends on: - MINOR: server/event_hdl: add server_inetaddr struct to facilitate event data usage - MINOR: server/event_hdl: update _srv_event_hdl_prepare_inetaddr prototype This should be backported in 2.9 with 683b2ae01 ("MINOR: server/event_hdl: add SERVER_INETADDR event") (cherry picked from commit 545e72546c0d729fd3de0f753adc905dd0b13908) Signed-off-by: Christopher Faulet --- include/haproxy/server-t.h | 5 ++++- src/server.c | 48 ++++++++++++++++++++++++++++++++------------ 2 files changed, 39 insertions(+), 14 deletions(-) diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h index 7d1cc70..9a9b48b 100644 --- a/include/haproxy/server-t.h +++ b/include/haproxy/server-t.h @@ -606,7 +606,10 @@ struct server_inetaddr { struct in_addr v4; struct in6_addr v6; } addr; /* may hold v4 or v6 addr */ - unsigned int svc_port; + struct { + unsigned int svc; + uint8_t map; /* is a mapped port? (boolean) */ + } port; }; /* data provided to EVENT_HDL_SUB_SERVER_INETADDR handlers through diff --git a/src/server.c b/src/server.c index 9228b45..b62dd06 100644 --- a/src/server.c +++ b/src/server.c @@ -173,10 +173,17 @@ int srv_getinter(const struct check *check) * Must be called under thread isolation to ensure consistent readings accross * all threads (addr:svc_port might be read without srv lock being held). */ -void _srv_set_inetaddr(struct server *srv, const struct sockaddr_storage *addr, unsigned int svc_port) +static void _srv_set_inetaddr_port(struct server *srv, + const struct sockaddr_storage *addr, + unsigned int svc_port, uint8_t mapped_port) { ipcpy(addr, &srv->addr); srv->svc_port = svc_port; + if (mapped_port) + srv->flags |= SRV_F_MAPPORTS; + else + srv->flags &= ~SRV_F_MAPPORTS; + if (srv->log_target && srv->log_target->type == LOG_TARGET_DGRAM) { /* server is used as a log target, manually update log target addr for DGRAM */ ipcpy(addr, srv->log_target->addr); @@ -184,6 +191,14 @@ void _srv_set_inetaddr(struct server *srv, const struct sockaddr_storage *addr, } } +/* same as _srv_set_inetaddr_port() but only updates the addr part + */ +static void _srv_set_inetaddr(struct server *srv, + const struct sockaddr_storage *addr) +{ + _srv_set_inetaddr_port(srv, addr, srv->svc_port, !!(srv->flags & SRV_F_MAPPORTS)); +} + /* * Function executed by server_atomic_sync_task to perform atomic updates on * compatible server struct members that are not guarded by any lock since @@ -288,7 +303,8 @@ static struct task *server_atomic_sync(struct task *task, void *context, unsigne thread_isolate_full(); /* apply new addr:port combination */ - _srv_set_inetaddr(srv, &new_addr, data->safe.next.svc_port); + _srv_set_inetaddr_port(srv, &new_addr, + data->safe.next.port.svc, data->safe.next.port.map); /* propagate the changes */ if (data->safe.purge_conn) /* force connection cleanup on the given server? */ @@ -422,11 +438,12 @@ void _srv_event_hdl_prepare_state(struct event_hdl_cb_data_server_state *cb_data static void _srv_event_hdl_prepare_inetaddr(struct event_hdl_cb_data_server_inetaddr *cb_data, struct server *srv, const struct sockaddr_storage *next_addr, - unsigned int next_port, + unsigned int next_port, uint8_t next_mapports, uint8_t purge_conn) { struct sockaddr_storage *prev_addr = &srv->addr; unsigned int prev_port = srv->svc_port; + uint8_t prev_mapports = !!(srv->flags & SRV_F_MAPPORTS); /* only INET families are supported */ BUG_ON((prev_addr->ss_family != AF_UNSPEC && @@ -444,7 +461,8 @@ static void _srv_event_hdl_prepare_inetaddr(struct event_hdl_cb_data_server_inet memcpy(&cb_data->safe.prev.addr.v6, &((struct sockaddr_in6 *)prev_addr)->sin6_addr, sizeof(struct in6_addr)); - cb_data->safe.prev.svc_port = prev_port; + cb_data->safe.prev.port.svc = prev_port; + cb_data->safe.prev.port.map = prev_mapports; /* next */ cb_data->safe.next.family = next_addr->ss_family; @@ -456,7 +474,8 @@ static void _srv_event_hdl_prepare_inetaddr(struct event_hdl_cb_data_server_inet memcpy(&cb_data->safe.next.addr.v6, &((struct sockaddr_in6 *)next_addr)->sin6_addr, sizeof(struct in6_addr)); - cb_data->safe.next.svc_port = next_port; + cb_data->safe.next.port.svc = next_port; + cb_data->safe.next.port.map = next_mapports; cb_data->safe.purge_conn = purge_conn; } @@ -3771,7 +3790,9 @@ int srv_update_addr(struct server *s, void *ip, int ip_sin_family, const char *u }; _srv_event_hdl_prepare(&cb_data.common, s, 0); - _srv_event_hdl_prepare_inetaddr(&cb_data.addr, s, &new_addr, s->svc_port, 0); + _srv_event_hdl_prepare_inetaddr(&cb_data.addr, s, + &new_addr, s->svc_port, !!(s->flags & SRV_F_MAPPORTS), + 0); /* server_atomic_sync_task will apply the changes for us */ _srv_event_hdl_publish(EVENT_HDL_SUB_SERVER_INETADDR, cb_data, s); @@ -3910,6 +3931,7 @@ const char *srv_update_addr_port(struct server *s, const char *addr, const char struct buffer *msg; int ip_change = 0; int port_change = 0; + uint8_t mapports = !!(s->flags & SRV_F_MAPPORTS); msg = get_trash_chunk(); chunk_reset(msg); @@ -4005,17 +4027,17 @@ const char *srv_update_addr_port(struct server *s, const char *addr, const char chunk_appendf(msg, "%d' to '", current_port); if (sign == '-') { - s->flags |= SRV_F_MAPPORTS; + mapports = 1; chunk_appendf(msg, "%c", sign); /* just use for result output */ new_port_print = -new_port_print; } else if (sign == '+') { - s->flags |= SRV_F_MAPPORTS; + mapports = 1; chunk_appendf(msg, "%c", sign); } else { - s->flags &= ~SRV_F_MAPPORTS; + mapports = 0; } chunk_appendf(msg, "%d'", new_port_print); @@ -4030,7 +4052,7 @@ out: _srv_event_hdl_prepare(&cb_data.common, s, 0); _srv_event_hdl_prepare_inetaddr(&cb_data.addr, s, ((ip_change) ? &sa : &s->addr), - ((port_change) ? new_port : s->svc_port), + ((port_change) ? new_port : s->svc_port), mapports, 1); /* server_atomic_sync_task will apply the changes for us */ @@ -4447,7 +4469,7 @@ int srv_set_addr_via_libc(struct server *srv, int *err_code) *err_code |= ERR_WARN; return 1; } - _srv_set_inetaddr(srv, &new_addr, srv->svc_port); + _srv_set_inetaddr(srv, &new_addr); return 0; } @@ -4534,7 +4556,7 @@ static int srv_apply_lastaddr(struct server *srv, int *err_code) *err_code |= ERR_WARN; return 1; } - _srv_set_inetaddr(srv, &new_addr, srv->svc_port); + _srv_set_inetaddr(srv, &new_addr); return 0; } @@ -4596,7 +4618,7 @@ static int srv_iterate_initaddr(struct server *srv) return return_code; case SRV_IADDR_IP: - _srv_set_inetaddr(srv, &srv->init_addr, srv->svc_port); + _srv_set_inetaddr(srv, &srv->init_addr); if (return_code) { ha_warning("could not resolve address '%s', falling back to configured address.\n", name); -- 1.7.10.4