From: Willy Tarreau Date: Tue, 15 Jun 2021 13:03:19 +0000 (+0200) Subject: BUG/MEDIUM: shctx: use at least thread-based locking on USE_PRIVATE_CACHE X-Git-Tag: v2.5-dev1~126 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=9e467af804b3dfa05eaa4677644f5ff0c0e0619f;p=thirdparty%2Fhaproxy.git 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 20d288769d..533536be8c 100644 --- 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 #endif -#include +#include +#include #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 050dbc3abf..4f8c23f7f8 100644 --- a/include/haproxy/shctx.h +++ b/include/haproxy/shctx.h @@ -14,7 +14,7 @@ #ifndef __HAPROXY_SHCTX_H #define __HAPROXY_SHCTX_H -#include +#include #include #include @@ -28,6 +28,8 @@ #include #endif #endif +#else +#include #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 b1fed39b49..b64e2bb24f 100644 --- 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 1160a1b253..0ac3d5e838 100644 --- a/src/shctx.c +++ b/src/shctx.c @@ -17,12 +17,8 @@ #include #include -#if !defined (USE_PRIVATE_CACHE) - int use_shared_mem = 0; -#endif - /* * Reserve a new row if is null, put it in the hotlist, set the refcount to 1 * or append new blocks to the row with 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);