MINOR: queue: add a function to check for TOCTOU after queueing

There's a rare TOCTOU case that happens from time to time with maxconn 1
and multiple threads. Between the moment we see the queue full and the
moment we queue a request, it's possible that the last request on the
server or proxy ended and that no other one is left to offer it its place.

Given that all this code path is performance-critical and we cannot afford
to increase the lock duration, better recheck for the condition after
queueing. For this we need to be able to check for the condition and
cleanly dequeue a request. That's what this patch provides via the new
function pendconn_must_try_again(). It will catch more requests than
absolutely needed though it will catch them all. It may find that around
1/1000 of requests are at risk, though testing shows that in practice,
it's around 1 per million that really gets stuck (other ones benefit
from timing and finishing late requests). Maybe in the future some
conditions might be refined but it's harmless.

What happens to such requests is that they're dequeued and their pendconn
freed, so that the caller can decide to try to LB or queue them again. For
now the function is not used, it's just added separately for easier tracking.

diff --git a/include/haproxy/queue.h b/include/haproxy/queue.h
index e77370cddd156d0a352748ce6ce957dd11ad6251..e4201fb174240427a4503d0e8fbd5336b7751a97 100644 (file)
--- a/include/haproxy/queue.h
+++ b/include/haproxy/queue.h
@@ -39,6 +39,7 @@ unsigned int srv_dynamic_maxconn(const struct server *s);
 int pendconn_redistribute(struct server *s);
 int pendconn_grab_from_px(struct server *s);
 void pendconn_unlink(struct pendconn *p);
+int pendconn_must_try_again(struct pendconn *p);
 
 /* Removes the pendconn from the server/proxy queue. It supports being called
  * with NULL for pendconn and with a pendconn not in the list. It is the

diff --git a/src/queue.c b/src/queue.c
index e55bb58982c1da0749e1a98fc7d5567c4a4ecdaa..bf32d4239806899ec2371dba31b9208aae6e4860 100644 (file)
--- a/src/queue.c
+++ b/src/queue.c
@@ -621,6 +621,68 @@ int pendconn_dequeue(struct stream *strm)
        return 0;
 }
 
+/* checks after a successful pendconn_add() if the connection ended up being
+ * alone with no active connection left to dequeue it. In such a case it will
+ * simply remove it from the queue, free it and return non-zero to inform the
+ * caller that it must try to add the connection again, otherwise it returns
+ * zero, indicating that the connection will be handled normally. The caller
+ * might have to drop SF_DIRECT and/or SF_ASSIGNED if the conn was on a proxy.
+ */
+int pendconn_must_try_again(struct pendconn *p)
+{
+       struct queue  *q  = p->queue;
+       struct proxy  *px = q->px;
+       struct server *sv = q->sv;
+       int ret = 0;
+
+       if (likely(!HA_ATOMIC_LOAD(&p->node.node.leaf_p)))
+               goto leave;
+
+       /* for a server, we need at least one conn left on this server to
+        * find ours.
+        */
+       if (likely(sv && HA_ATOMIC_LOAD(&sv->served)))
+               goto leave;
+
+       /* for a backend, we need at least one conn left on any of this
+        * backend's servers to find ours.
+        */
+       if (likely(!sv && HA_ATOMIC_LOAD(&px->served)))
+               goto leave;
+
+       /* OK the situation is not safe anymore, we need to check if we're
+        * still in the queue under a lock.
+        */
+       HA_SPIN_LOCK(QUEUE_LOCK, &q->lock);
+       HA_SPIN_LOCK(QUEUE_LOCK, &p->del_lock);
+
+       if (p->node.node.leaf_p) {
+               eb32_delete(&p->node);
+               _HA_ATOMIC_DEC(&q->length);
+               _HA_ATOMIC_INC(&q->idx);
+               _HA_ATOMIC_DEC(&px->totpend);
+               ret = 1;
+       }
+
+       HA_SPIN_UNLOCK(QUEUE_LOCK, &p->del_lock);
+       HA_SPIN_UNLOCK(QUEUE_LOCK, &q->lock);
+
+       /* check if the connection was still queued. If not, it means its
+        * processing has begun so it's safe.
+        */
+       if (!ret)
+               goto leave;
+
+       /* The pendconn is not queued anymore and will not be so we're safe
+        * to free it.
+        */
+       p->strm->pend_pos = NULL;
+       pool_free(pool_head_pendconn, p);
+
+leave:
+       return ret;
+}
+
 static enum act_return action_set_priority_class(struct act_rule *rule, struct proxy *px,
                                                  struct session *sess, struct stream *s, int flags)
 {