BUG/MEDIUM: memory: Add a rwlock before freeing memory.

When using lockless pools, add a new rwlock, flush_pool. read-lock it when
getting memory from the pool, so that concurrenct access are still
authorized, but write-lock it when we're about to free memory, in
pool_flush() and pool_gc().
The problem is, when removing an item from the pool, we unreference it
to get the next one, however, that pointer may have been free'd in the
meanwhile, and that could provoke a crash if the pointer has been unmapped.
It should be OK to use a rwlock, as normal operations will still be able
to access the pool concurrently, and calls to pool_flush() and pool_gc()
should be pretty rare.

This should be backported to 2.1, 2.0 and 1.9.

diff --git a/include/common/memory.h b/include/common/memory.h
index 1aab6d460bd6538d43535be1c9ba0e6e7df3aa59..cafe03aca46296b4e15f93d92e81d492b1f648a7 100644 (file)
--- a/include/common/memory.h
+++ b/include/common/memory.h
@@ -78,6 +78,7 @@ struct pool_head {
        void **free_list;
 #ifdef CONFIG_HAP_LOCKLESS_POOLS
        uintptr_t seq;
+       HA_RWLOCK_T flush_lock;
 #else
        __decl_hathreads(HA_SPINLOCK_T lock); /* the spin lock */
 #endif
@@ -221,6 +222,7 @@ static inline void *__pool_get_first(struct pool_head *pool)
        cmp.seq = pool->seq;
        __ha_barrier_load();
 
+       HA_RWLOCK_RDLOCK(POOL_LOCK, &pool->flush_lock);
        cmp.free_list = pool->free_list;
        do {
                if (cmp.free_list == NULL)
@@ -230,6 +232,7 @@ static inline void *__pool_get_first(struct pool_head *pool)
                new.free_list = *POOL_LINK(pool, cmp.free_list);
        } while (HA_ATOMIC_DWCAS((void *)&pool->free_list, (void *)&cmp, (void *)&new) == 0);
        __ha_barrier_atomic_store();
+       HA_RWLOCK_RDUNLOCK(POOL_LOCK, &pool->flush_lock);
 
        _HA_ATOMIC_ADD(&pool->used, 1);
 #ifdef DEBUG_MEMORY_POOLS

diff --git a/src/memory.c b/src/memory.c
index adc293874c2712c51e710cd22753490c7fdbec5d..d1aec5925c327aa7c93e51c5e152b8fa41e5f11b 100644 (file)
--- a/src/memory.c
+++ b/src/memory.c
@@ -141,6 +141,8 @@ struct pool_head *create_pool(char *name, unsigned int size, unsigned int flags)
                }
 #ifndef CONFIG_HAP_LOCKLESS_POOLS
                HA_SPIN_INIT(&pool->lock);
+#else
+               HA_RWLOCK_INIT(&pool->flush_lock);
 #endif
        }
        pool->users++;
@@ -223,6 +225,7 @@ void pool_flush(struct pool_head *pool)
 
        if (!pool)
                return;
+       HA_RWLOCK_WRLOCK(POOL_LOCK, &pool->flush_lock);
        do {
                cmp.free_list = pool->free_list;
                cmp.seq = pool->seq;
@@ -230,6 +233,7 @@ void pool_flush(struct pool_head *pool)
                new.seq = cmp.seq + 1;
        } while (!_HA_ATOMIC_DWCAS(&pool->free_list, &cmp, &new));
        __ha_barrier_atomic_store();
+       HA_RWLOCK_WRUNLOCK(POOL_LOCK, &pool->flush_lock);
        next = cmp.free_list;
        while (next) {
                temp = next;
@@ -259,6 +263,7 @@ void pool_gc(struct pool_head *pool_ctx)
                return;
 
        list_for_each_entry(entry, &pools, list) {
+               HA_RWLOCK_WRLOCK(POOL_LOCK, &entry->flush_lock);
                while ((int)((volatile int)entry->allocated - (volatile int)entry->used) > (int)entry->minavail) {
                        struct pool_free_list cmp, new;
 
@@ -275,6 +280,7 @@ void pool_gc(struct pool_head *pool_ctx)
                        free(cmp.free_list);
                        _HA_ATOMIC_SUB(&entry->allocated, 1);
                }
+               HA_RWLOCK_WRUNLOCK(POOL_LOCK, &entry->flush_lock);
        }
 
        _HA_ATOMIC_STORE(&recurse, 0);
@@ -629,7 +635,7 @@ int mem_should_fail(const struct pool_head *pool)
                else
                        ret = 0;
        }
-       HA_SPIN_LOCK(OTHER_LOCK, &mem_fail_lock);
+       HA_SPIN_LOCK(POOL_LOCK, &mem_fail_lock);
        n = snprintf(&mem_fail_str[mem_fail_cur_idx * MEM_FAIL_MAX_CHAR],
            MEM_FAIL_MAX_CHAR - 2,
            "%d %.18s %d %d", mem_fail_cur_idx, pool->name, ret, tid);
@@ -642,7 +648,7 @@ int mem_should_fail(const struct pool_head *pool)
        mem_fail_cur_idx++;
        if (mem_fail_cur_idx == MEM_FAIL_MAX_STR)
                mem_fail_cur_idx = 0;
-       HA_SPIN_UNLOCK(OTHER_LOCK, &mem_fail_lock);
+       HA_SPIN_UNLOCK(POOL_LOCK, &mem_fail_lock);
        return ret;
 
 }