BUG/MEDIUM: shctx: use at least thread-based locking on USE_PRIVATE_CACHE

Since threads were introduced in 1.8, the USE_PRIVATE_CACHE mode of the
shctx was not updated to use locks. Originally it was meant to disable
sharing between processes, so it removes the lock/unlock instructions.
But with threads enabled, it's not possible to work like this anymore.

It's easy to see that once built with private cache and threads enabled,
sending violent SSL traffic to the the process instantly makes it die.
The HTTP cache is very likely affected as well.

This patch addresses this by falling back to our native spinlocks when
USE_PRIVATE_CACHE is used. In practice we could use them also for other
modes and remove all older implementations, but this patch aims at keeping
the changes very low and easy to backport. A new SHCTX_LOCK label was
added to help with debugging, but OTHER_LOCK might be usable as well
for backports.

An even lighter approach for backports may consist in always declaring
the lock (or reusing "waiters"), and calling pl_take_s() for the lock()
and pl_drop_s() for the unlock() operation. This could even be used in
all modes (process and threads), even when thread support is disabled.

Subsequent patches will further clean up this area.

This patch must be backported to all supported versions since 1.8.

diff --git a/include/haproxy/shctx-t.h b/include/haproxy/shctx-t.h
index 20d288769d002be4007af6495d541ab6022b486f..533536be8c6c177bdafb10b5262b07140c74316c 100644 (file)
--- a/include/haproxy/shctx-t.h
+++ b/include/haproxy/shctx-t.h
@@ -17,7 +17,8 @@
 #if !defined (USE_PRIVATE_CACHE) && defined(USE_PTHREAD_PSHARED)
 #include <pthread.h>
 #endif
-#include <haproxy/list.h>
+#include <haproxy/api-t.h>
+#include <haproxy/thread-t.h>
 
 #ifndef SHSESS_BLOCK_MIN_SIZE
 #define SHSESS_BLOCK_MIN_SIZE 128
@@ -54,6 +55,8 @@ struct shared_context {
 #else
        unsigned int waiters;
 #endif
+#else
+       __decl_thread(HA_SPINLOCK_T lock);  // used when USE_PRIVATE_CACHE=1
 #endif
        struct list avail;  /* list for active and free blocks */
        struct list hot;     /* list for locked blocks */

diff --git a/include/haproxy/shctx.h b/include/haproxy/shctx.h
index 050dbc3abf31b4a62aee126df4013a2801d2dd00..4f8c23f7f8ff4b4440e17e14836c49d078aadfa5 100644 (file)
--- a/include/haproxy/shctx.h
+++ b/include/haproxy/shctx.h
@@ -14,7 +14,7 @@
 #ifndef __HAPROXY_SHCTX_H
 #define __HAPROXY_SHCTX_H
 
-#include <haproxy/api-t.h>
+#include <haproxy/api.h>
 #include <haproxy/list.h>
 #include <haproxy/shctx-t.h>
 
@@ -28,6 +28,8 @@
 #include <sys/syscall.h>
 #endif
 #endif
+#else
+#include <haproxy/thread.h>
 #endif
 
 int shctx_init(struct shared_context **orig_shctx,
@@ -47,9 +49,10 @@ int shctx_row_data_get(struct shared_context *shctx, struct shared_block *first,
 /* Lock functions */
 
 #if defined (USE_PRIVATE_CACHE)
+extern int use_shared_mem;
 
-#define shctx_lock(shctx)
-#define shctx_unlock(shctx)
+#define shctx_lock(shctx)   if (use_shared_mem) HA_SPIN_LOCK(SHCTX_LOCK, &shctx->lock)
+#define shctx_unlock(shctx) if (use_shared_mem) HA_SPIN_UNLOCK(SHCTX_LOCK, &shctx->lock)
 
 #elif defined (USE_PTHREAD_PSHARED)
 extern int use_shared_mem;

diff --git a/include/haproxy/thread.h b/include/haproxy/thread.h
index b1fed39b491f132598d4fde566eb6a501e745c75..b64e2bb24f64c224183794bbe11b7c0f6bac7769 100644 (file)
--- a/include/haproxy/thread.h
+++ b/include/haproxy/thread.h
@@ -380,6 +380,7 @@ enum lock_label {
        STK_SESS_LOCK,
        APPLETS_LOCK,
        PEER_LOCK,
+       SHCTX_LOCK,
        SSL_LOCK,
        SSL_GEN_CERTS_LOCK,
        PATREF_LOCK,
@@ -431,6 +432,7 @@ static inline const char *lock_label(enum lock_label label)
        case STK_SESS_LOCK:        return "STK_SESS";
        case APPLETS_LOCK:         return "APPLETS";
        case PEER_LOCK:            return "PEER";
+       case SHCTX_LOCK:           return "SHCTX";
        case SSL_LOCK:             return "SSL";
        case SSL_GEN_CERTS_LOCK:   return "SSL_GEN_CERTS";
        case PATREF_LOCK:          return "PATREF";

diff --git a/src/shctx.c b/src/shctx.c
index 1160a1b2531ca0f7e2c7e3240d6425f7f2b17643..0ac3d5e8384202609361034ef12832fdf21657e3 100644 (file)
--- a/src/shctx.c
+++ b/src/shctx.c
@@ -17,12 +17,8 @@
 #include <haproxy/list.h>
 #include <haproxy/shctx.h>
 
-#if !defined (USE_PRIVATE_CACHE)
-
 int use_shared_mem = 0;
 
-#endif
-
 /*
  * Reserve a new row if <first> is null, put it in the hotlist, set the refcount to 1
  * or append new blocks to the row with <first> as first block if non null.
@@ -296,10 +292,8 @@ int shctx_init(struct shared_context **orig_shctx, int maxblocks, int blocksize,
        int i;
        struct shared_context *shctx;
        int ret;
-#ifndef USE_PRIVATE_CACHE
 #ifdef USE_PTHREAD_PSHARED
        pthread_mutexattr_t attr;
-#endif
 #endif
        void *cur;
        int maptype = MAP_PRIVATE;
@@ -311,10 +305,8 @@ int shctx_init(struct shared_context **orig_shctx, int maxblocks, int blocksize,
        blocksize = (blocksize + sizeof(void *) - 1) & -sizeof(void *);
        extra     = (extra     + sizeof(void *) - 1) & -sizeof(void *);
 
-#ifndef USE_PRIVATE_CACHE
        if (shared)
                maptype = MAP_SHARED;
-#endif
 
        shctx = (struct shared_context *)mmap(NULL, sizeof(struct shared_context) + extra + (maxblocks * (sizeof(struct shared_block) + blocksize)),
                                              PROT_READ | PROT_WRITE, maptype | MAP_ANON, -1, 0);
@@ -326,8 +318,8 @@ int shctx_init(struct shared_context **orig_shctx, int maxblocks, int blocksize,
 
        shctx->nbav = 0;
 
-#ifndef USE_PRIVATE_CACHE
        if (maptype == MAP_SHARED) {
+#ifndef USE_PRIVATE_CACHE
 #ifdef USE_PTHREAD_PSHARED
                if (pthread_mutexattr_init(&attr)) {
                        munmap(shctx, sizeof(struct shared_context) + extra + (maxblocks * (sizeof(struct shared_block) + blocksize)));
@@ -353,10 +345,12 @@ int shctx_init(struct shared_context **orig_shctx, int maxblocks, int blocksize,
                }
 #else
                shctx->waiters = 0;
+#endif
+#else
+               HA_SPIN_INIT(&shctx->lock);
 #endif
                use_shared_mem = 1;
        }
-#endif
 
        LIST_INIT(&shctx->avail);
        LIST_INIT(&shctx->hot);