From 7d00077fd5bd21e13aa976e6f3221cd44ae05eea Mon Sep 17 00:00:00 2001 From: Aurelien DARRAGON Date: Thu, 8 Sep 2022 14:35:35 +0200 Subject: [PATCH] BUG/MEDIUM: proxy: ensure pause_proxy() and resume_proxy() own PROXY_LOCK There was a race involving hlua_proxy_* functions and some proxy management functions. pause_proxy() and resume_proxy() can be used directly from lua code, but that could lead to some race as lua code didn't make sure PROXY_LOCK was owned before calling the proxy functions. This patch makes sure it won't happen again elsewhere in the code by locking PROXY_LOCK directly in resume and pause proxy functions so that it's not the caller's responsibility anymore. (based on stop_proxy() behavior that was already safe prior to the patch) This should be backported to stable series. Note that the API will likely differ < 2.4 --- src/hlua_fcn.c | 3 +++ src/proxy.c | 28 +++++++++++++++++++++------- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/hlua_fcn.c b/src/hlua_fcn.c index 5907d48557..0046e9ede4 100644 --- a/src/hlua_fcn.c +++ b/src/hlua_fcn.c @@ -1311,6 +1311,7 @@ int hlua_proxy_pause(lua_State *L) struct proxy *px; px = hlua_check_proxy(L, 1); + /* safe to call without PROXY_LOCK - pause_proxy takes it */ pause_proxy(px); return 0; } @@ -1320,6 +1321,7 @@ int hlua_proxy_resume(lua_State *L) struct proxy *px; px = hlua_check_proxy(L, 1); + /* safe to call without PROXY_LOCK - resume_proxy takes it */ resume_proxy(px); return 0; } @@ -1329,6 +1331,7 @@ int hlua_proxy_stop(lua_State *L) struct proxy *px; px = hlua_check_proxy(L, 1); + /* safe to call without PROXY_LOCK - stop_proxy takes it */ stop_proxy(px); return 0; } diff --git a/src/proxy.c b/src/proxy.c index 6fad8ee91b..7b1c96eb43 100644 --- a/src/proxy.c +++ b/src/proxy.c @@ -2252,13 +2252,17 @@ void soft_stop(void) /* Temporarily disables listening on all of the proxy's listeners. Upon * success, the proxy enters the PR_PAUSED state. The function returns 0 * if it fails, or non-zero on success. + * The function takes the proxy's lock so it's safe to + * call from multiple places. */ int pause_proxy(struct proxy *p) { struct listener *l; + HA_RWLOCK_WRLOCK(PROXY_LOCK, &p->lock); + if (!(p->cap & PR_CAP_FE) || (p->flags & (PR_FL_DISABLED|PR_FL_STOPPED)) || !p->li_ready) - return 1; + goto end; list_for_each_entry(l, &p->conf.listeners, by_fe) pause_listener(l); @@ -2266,8 +2270,11 @@ int pause_proxy(struct proxy *p) if (p->li_ready) { ha_warning("%s %s failed to enter pause mode.\n", proxy_cap_str(p->cap), p->id); send_log(p, LOG_WARNING, "%s %s failed to enter pause mode.\n", proxy_cap_str(p->cap), p->id); + HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &p->lock); return 0; } +end: + HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &p->lock); return 1; } @@ -2276,7 +2283,8 @@ int pause_proxy(struct proxy *p) * to be called when going down in order to release the ports so that another * process may bind to them. It must also be called on disabled proxies at the * end of start-up. If all listeners are closed, the proxy is set to the - * PR_STOPPED state. The function takes the proxy's lock so it's safe to + * PR_STOPPED state. + * The function takes the proxy's lock so it's safe to * call from multiple places. */ void stop_proxy(struct proxy *p) @@ -2300,14 +2308,18 @@ void stop_proxy(struct proxy *p) * listeners and tries to enable them all. If any of them fails, the proxy is * put back to the paused state. It returns 1 upon success, or zero if an error * is encountered. + * The function takes the proxy's lock so it's safe to + * call from multiple places. */ int resume_proxy(struct proxy *p) { struct listener *l; int fail; + HA_RWLOCK_WRLOCK(PROXY_LOCK, &p->lock); + if ((p->flags & (PR_FL_DISABLED|PR_FL_STOPPED)) || !p->li_paused) - return 1; + goto end; fail = 0; list_for_each_entry(l, &p->conf.listeners, by_fe) { @@ -2335,9 +2347,13 @@ int resume_proxy(struct proxy *p) } if (fail) { + HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &p->lock); + /* pause_proxy will take PROXY_LOCK */ pause_proxy(p); return 0; } +end: + HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &p->lock); return 1; } @@ -3051,9 +3067,8 @@ static int cli_parse_disable_frontend(char **args, char *payload, struct appctx if (!px->li_ready) return cli_msg(appctx, LOG_NOTICE, "All sockets are already disabled.\n"); - HA_RWLOCK_WRLOCK(PROXY_LOCK, &px->lock); + /* pause_proxy will take PROXY_LOCK */ ret = pause_proxy(px); - HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &px->lock); if (!ret) return cli_err(appctx, "Failed to pause frontend, check logs for precise cause.\n"); @@ -3083,9 +3098,8 @@ static int cli_parse_enable_frontend(char **args, char *payload, struct appctx * if (px->li_ready == px->li_all) return cli_msg(appctx, LOG_NOTICE, "All sockets are already enabled.\n"); - HA_RWLOCK_WRLOCK(PROXY_LOCK, &px->lock); + /* resume_proxy will take PROXY_LOCK */ ret = resume_proxy(px); - HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &px->lock); if (!ret) return cli_err(appctx, "Failed to resume frontend, check logs for precise cause (port conflict?).\n"); -- 2.39.5