From: Willy Tarreau Date: Wed, 8 Jan 2020 18:15:07 +0000 (+0100) Subject: BUG/MAJOR: listener: do not schedule a task-less proxy X-Git-Tag: v2.2-dev1~125 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=eeea8082a8d7b1b4490caf6564228ee18f154bca;p=thirdparty%2Fhaproxy.git BUG/MAJOR: listener: do not schedule a task-less proxy Apparently seamingless commit 0591bf7deb ("MINOR: listener: make the wait paths cleaner and more reliable") caused a nasty regression and revealed a rare race that hits regtest stickiness/lb-services.vtc about 4% of the times for 8 threads. The problem is that when a multi-threaded listener wakes up on an incoming connection, several threads can receive the event, especially when idle. And all of them will race to accept the connections in parallel, adjusting the listener's nbconn and proxy's feconn until one reaches the proxy's limit and declines. At this step the changes are cancelled, the listener is marked "limited", and when the threads exit the function, one of them will unlimit the listener/proxy again so that it can accept incoming connections again. The problem happens when many threads connect to a small peers section because its maxconn is very limited (typically 6 for 2 peers), and it's sometimes possible for enough competing threads to hit the limit and one of them will limit the listener and queue the proxy's task... except that peers do not initialize their proxy task since they do not use rate limiting. Thus the process crashes when doing task_schedule(p->task). Prior to the cleanup patch above, this didn't happen because the error path that was dedicated to only limiting the listener did not call task_schedule(p->task). Given that the proxy's task is optional, and that the expire value passed there is always TICK_ETERNITY, it's sufficient and reasonable to avoid calling this task_schedule() when expire is not set. And for long term safety we can also avoid to do it when the task is not set. A first fix consisted in allocating a task for the peers proxies but it's never used and would eat resources for reason. No backport is needed as this commit was only merged into 2.2. --- diff --git a/src/listener.c b/src/listener.c index faadbea749..5b2495db28 100644 --- a/src/listener.c +++ b/src/listener.c @@ -1102,7 +1102,8 @@ void listener_accept(int fd) * later than ahead. The listener turns to LI_LIMITED. */ limit_listener(l, &p->listener_queue); - task_schedule(p->task, expire); + if (p->task && tick_isset(expire)) + task_schedule(p->task, expire); goto end; }