Assert if slab elements are not released correctly

diff --git a/src/lib/util/slab.h b/src/lib/util/slab.h
index d2320874ef4975cf7ff6e4a020e56f347a6e6ad7..0f2c2eb0f32433205fc0a3a884c6d3856d931398 100644 (file)
--- a/src/lib/util/slab.h
+++ b/src/lib/util/slab.h
@@ -47,6 +47,9 @@ typedef struct { \
                                                        ///< number in use has reached max_elements.
        unsigned int            num_children;           //!< How many child allocations are expected off each element.
        size_t                  child_pool_size;        //!< Size of pool space to be allocated to each element.
+       bool                    allow_direct_free;      //!< Allow in-use elements to be freed with talloc_free.
+                                                       ///< Normally this is a programming error and triggers an
+                                                       ///< assertion.  Only set for testing the destructor path.
        fr_time_delta_t         interval;               //!< Interval between slab cleanup events being fired.
 } fr_slab_config_t;
 
@@ -96,6 +99,7 @@ typedef struct { \
                FR_DLIST_ENTRY(_name ## _slab)          entry; \
                _name ## _slab_list_t                   *list; \
                TALLOC_CTX                              *pool; \
+               bool                                    being_freed; \
                FR_DLIST_HEAD(_name ## _slab_element)   reserved; \
                FR_DLIST_HEAD(_name ## _slab_element)   avail; \
        } _name ## _slab_t; \
@@ -203,12 +207,31 @@ DIAG_OFF(unused-function) \
                } \
                return slab; \
        } \
+\
+       /** Callback for talloc freeing a slab \
+        * \
+        * Sets the being_freed flag so that element destructors know this \
+        * is a bulk teardown and not an erroneous direct free. \
+        * talloc calls this destructor before freeing children. \
+        */ \
+       static int _ ## _name ## _slab_free(_name ## _slab_t *slab) \
+       { \
+               slab->being_freed = true; \
+               return 0; \
+       } \
 \
        /** Callback for talloc freeing a slab element \
         * \
-        * Ensure that the element reset and custom destructor is called even if \
-        * the element is not released before being freed. \
-        * Also ensure the element is removed from the relevant list. \
+        * Called during bulk teardown (talloc_free of a slab or slab list). \
+        * Ensures the element is cleanly removed from whichever dlist it is on. \
+        * \
+        * In-use slab elements must NEVER be talloc_free'd directly.  Use \
+        * the type-specific _slab_release() function instead.  Calling \
+        * talloc_free permanently frees the element's pool memory,
+        * progressively degrading slab efficiency.  The talloc pool cannot \
+        * be fully reclaimed until ALL children are freed, so piecemeal frees \
+        * fragment the pool and shrink the usable element count on each reallocation
+        * cycle. \
         */ \
        static int _ ## _type ## _element_free(_name ## _slab_element_t *element) \
        { \
@@ -217,6 +240,10 @@ DIAG_OFF(unused-function) \
                if (!element->slab) return 0; \
                slab = element->slab; \
                if (element->in_use) { \
+                       fr_assert_msg(slab->being_freed || slab->list->config.allow_direct_free, \
+                                     "in-use slab element freed with talloc_free - use _slab_release()"); \
+                       slab->list->in_use--; \
+                       element->in_use = false; \
                        _name ## _slab_element_remove(&slab->reserved, element); \
                } else { \
                        _name ## _slab_element_remove(&slab->avail, element); \
@@ -249,6 +276,7 @@ DIAG_OFF(unused-function) \
                        elem_size = slab_list->config.elements_per_slab * (sizeof(_name ## _slab_element_t) + \
                                                                           slab_list->config.child_pool_size); \
                        MEM(slab = talloc_zero_pooled_object(slab_list, _name ## _slab_t, elems, elem_size)); \
+                       talloc_set_destructor(slab, _ ## _name ## _slab_free); \
                        _name ## _slab_element_init(&slab->avail); \
                        _name ## _slab_element_init(&slab->reserved); \
                        _name ## _slab_insert_head(&slab_list->avail, slab); \

diff --git a/src/lib/util/test/slab_tests.c b/src/lib/util/test/slab_tests.c
index cc563a28735d5e14888febd6492109888e188603..760e00fe41709235bf32dad6ccb64720595524ec 100644 (file)
--- a/src/lib/util/test/slab_tests.c
+++ b/src/lib/util/test/slab_tests.c
@@ -317,15 +317,17 @@ static void test_reserve_mru(void)
 }
 
 /** Test that talloc freeing an element results in destructor being called
- *
+ *  and that accounting is correctly updated.
  */
 static void test_free(void)
 {
        test_slab_list_t        *test_slab_list;
        test_element_t          *test_element;
        test_uctx_t             test_uctx;
+       fr_slab_config_t        slab_config = def_slab_config;
 
-       test_slab_list = test_slab_list_alloc(NULL, NULL, &def_slab_config, NULL, NULL, NULL, true, false);
+       slab_config.allow_direct_free = true;
+       test_slab_list = test_slab_list_alloc(NULL, NULL, &slab_config, NULL, NULL, NULL, true, false);
        TEST_CHECK(test_slab_list != NULL);
        if (!test_slab_list) return;
 
@@ -337,8 +339,11 @@ static void test_free(void)
        if (test_element) test_element->name = talloc_strdup(test_element, "Hello there");
        if (test_element) test_slab_element_set_destructor(test_element, test_element_free, &test_uctx);
 
+       TEST_CHECK_RET(test_slab_num_elements_used(test_slab_list), 1);
+
        if (test_element) talloc_free(test_element);
        TEST_CHECK(test_uctx.count == 11);
+       TEST_CHECK_RET(test_slab_num_elements_used(test_slab_list), 0);
 
        talloc_free(test_slab_list);
 }

diff --git a/src/modules/rlm_cache/drivers/rlm_cache_memcached/rlm_cache_memcached.c b/src/modules/rlm_cache/drivers/rlm_cache_memcached/rlm_cache_memcached.c
index 52014aa5d13ea18b7e22a023a1ba31181d4933ec..63bd5154f7d0f381abc774386651f1635e9d14df 100644 (file)
--- a/src/modules/rlm_cache/drivers/rlm_cache_memcached/rlm_cache_memcached.c
+++ b/src/modules/rlm_cache/drivers/rlm_cache_memcached/rlm_cache_memcached.c
@@ -312,7 +312,7 @@ static int mod_conn_reconnect(void **handle, UNUSED rlm_cache_config_t const *co
        rlm_cache_memcached_thread_t    *t = talloc_get_type_abort(module_thread(driver->mi)->data, rlm_cache_memcached_thread_t);
        rlm_cache_handle_t              *mandle;
 
-       talloc_free(*handle);
+       memcached_slab_release(*handle);
        mandle = memcached_slab_reserve(t->slab);
        if (!mandle) {
                *handle = NULL;