core: fix memory pool management races

Objects allocated from a per-thread memory pool keep a reference to it
to be able to return the object to the pool when not used anymore. The
object holding this reference can have a long life cycle that could
survive a glfs_fini() call.

This means that it's unsafe to destroy memory pools from glfs_fini().

Another side effect of destroying memory pools from glfs_fini() is that
the TLS variable that points to one of those pools cannot be reset for
all alive threads.  This means that any attempt to allocate memory from
those threads will access already free'd memory, which is very
dangerous.

To fix these issues, mem_pools_fini() doesn't destroy pool lists
anymore. They should be destroyed when the library is unloaded or the
process is terminated, but this cannot be done right now because
gluster doesn't stop other threads before calling exit(), which could
cause some races.

This patch is the backport of 2 master patches:
> Change-Id: Ib189a5510ab6bdac78983c6c65a022e9634b0965
> Fixes: bz#1801684
> Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>
>
> Change-Id: Id7cfb4407fcf208e28f03a7c3cdc3ef9c1f3bf9b
> Fixes: bz#1801684
> Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>

Change-Id: Id7cfb4407fcf208e28f03a7c3cdc3ef9c1f3bf9b
Fixes: bz#1805671
Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>

2 files changed, 18 insertions, 13 deletions

diff --git a/libglusterfs/src/glusterfs/globals.h b/libglusterfs/src/glusterfs/globals.h
index 994301bd862..acb8e9a6b69 100644
--- a/libglusterfs/src/glusterfs/globals.h
+++ b/libglusterfs/src/glusterfs/globals.h
@@ -160,6 +160,9 @@ glusterfs_leaseid_exist(void);
 int
 glusterfs_globals_init(glusterfs_ctx_t *ctx);
 
+void
+gf_thread_needs_cleanup(void);
+
 struct tvec_base *
 glusterfs_ctx_tw_get(glusterfs_ctx_t *ctx);
 void
diff --git a/libglusterfs/src/glusterfs/mem-pool.h b/libglusterfs/src/glusterfs/mem-pool.h
index be0a26d3f64..97bf76c0da2 100644
--- a/libglusterfs/src/glusterfs/mem-pool.h
+++ b/libglusterfs/src/glusterfs/mem-pool.h
@@ -245,24 +245,26 @@ typedef struct per_thread_pool {
 } per_thread_pool_t;
 
 typedef struct per_thread_pool_list {
-    /*
-     * These first two members are protected by the global pool lock.  When
-     * a thread first tries to use any pool, we create one of these.  We
-     * link it into the global list using thr_list so the pool-sweeper
-     * thread can find it, and use pthread_setspecific so this thread can
-     * find it.  When the per-thread destructor runs, we "poison" the pool
-     * list to prevent further allocations.  This also signals to the
-     * pool-sweeper thread that the list should be detached and freed after
-     * the next time it's swept.
-     */
+    /* thr_list is used to place the TLS pool_list into the active global list
+     * (pool_threads) or the inactive global list (pool_free_threads). It's
+     * protected by the global pool_lock. */
     struct list_head thr_list;
-    unsigned int poison;
+
+    /* This lock is used to update poison and the hot/cold lists of members
+     * of 'pools' array. */
+    pthread_spinlock_t lock;
+
+    /* This field is used to mark a pool_list as not being owned by any thread.
+     * This means that the sweeper thread won't be cleaning objects stored in
+     * its pools. mem_put() uses it to decide if the object being released is
+     * placed into its original pool_list or directly destroyed. */
+    bool poison;
+
     /*
      * There's really more than one pool, but the actual number is hidden
      * in the implementation code so we just make it a single-element array
      * here.
      */
-    pthread_spinlock_t lock;
     per_thread_pool_t pools[1];
 } per_thread_pool_list_t;
 
@@ -307,7 +309,7 @@ void
 mem_pool_destroy(struct mem_pool *pool);
 
 void
-mem_pool_thread_destructor(void);
+mem_pool_thread_destructor(per_thread_pool_list_t *pool_list);
 
 void
 gf_mem_acct_enable_set(void *ctx);