[PATCH v1 0/3] mempool: fix races between list walk and create/free

[PATCH v1 0/3] mempool: fix races between list walk and create/free

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

A thread walking the mempool list with rte_mempool_walk(),
rte_mempool_lookup() or rte_mempool_list_dump() can crash while other
threads create and free mempools. We hit this in production, in a
process that walks the mempools while other threads create and free
them.

There are two races:

- rte_mempool_free() unlinks and frees the list entry without the
  mempool list lock that the walkers hold, so a walker can reach a
  freed entry or mempool. Patch 1 takes the lock in rte_mempool_free(),
  as rte_mempool_create_empty() already does. A rte_mempool_walk()
  callback can therefore no longer free a mempool; this is documented.

- A mempool is on the list before its store exists, and walking or
  dumping it reads the store. Patch 2 skips mempools without
  RTE_MEMPOOL_F_POOL_CREATED in rte_mempool_walk() and
  rte_mempool_list_dump(). The release/acquire fences it adds generate
  no instructions on x86.

Patch 3 adds a test to mempool_autotest. On x86 with 4 or 8 lcores,
it crashed in all 20 runs without patch 1, without patch 2, or
without both, and passed in all 20 runs with both patches.

Robert Sanford (3):
  mempool: fix race between list walk and free
  mempool: skip unpopulated pools in walk and dump
  test/mempool: add list walk race test

 app/test/test_mempool.c   | 137 ++++++++++++++++++++++++++++++++++++++
 lib/mempool/rte_mempool.c |  28 +++++++-
 lib/mempool/rte_mempool.h |   6 ++
 3 files changed, 170 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH v1 1/3] mempool: fix race between list walk and free

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

rte_mempool_walk(), rte_mempool_lookup() and rte_mempool_list_dump()
traverse the mempool list holding only the mempool list lock.
rte_mempool_create_empty() inserts into the list holding both the
mempool list lock and the tailq lock, but rte_mempool_free() unlinks
and frees the list entry holding only the tailq lock. So a mempool can
be unlinked and freed, together with its list entry, while another
thread is walking the list. The walker then reads freed memory. Since
freed heap memory is zeroed, the walk callback is passed a NULL
mempool, or rte_mempool_lookup() compares the name of one, and the
process crashes.

Take the mempool list lock in rte_mempool_free() as well, in the same
order as rte_mempool_create_empty(). As a result, a rte_mempool_walk()
callback must not free a mempool, as it already could not create one.
Document this.

Fixes: a3791ff0a3f8 ("mempool: introduce a function to free a pool")
Cc: stable@dpdk.org

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 lib/mempool/rte_mempool.c | 8 ++++++++
 lib/mempool/rte_mempool.h | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index 069aabaa70..d9f339a3c2 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -729,6 +729,13 @@ rte_mempool_free(struct rte_mempool *mp)
 		return;
 
 	mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempool_list);
+	/*
+	 * Take the mempool list lock as well, like rte_mempool_create_empty(),
+	 * so that the entry is not unlinked and freed while
+	 * rte_mempool_walk(), rte_mempool_lookup() or rte_mempool_list_dump()
+	 * is traversing the list.
+	 */
+	rte_mcfg_mempool_write_lock();
 	rte_mcfg_tailq_write_lock();
 	/* find out tailq entry */
 	TAILQ_FOREACH(te, mempool_list, next) {
@@ -741,6 +748,7 @@ rte_mempool_free(struct rte_mempool *mp)
 		rte_free(te);
 	}
 	rte_mcfg_tailq_write_unlock();
+	rte_mcfg_mempool_write_unlock();
 
 	mempool_event_callback_invoke(RTE_MEMPOOL_EVENT_DESTROY, mp);
 	rte_mempool_trace_free(mp);
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index b34cd893b1..bd58d8a9d5 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1947,6 +1947,9 @@ uint32_t rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
 /**
  * Walk list of all memory pools
  *
+ * The mempool list is locked while the iterator function runs, so the
+ * iterator function must not create or free a mempool.
+ *
  * @param func
  *   Iterator function
  * @param arg
-- 
2.34.1

[PATCH v1 2/3] mempool: skip unpopulated pools in walk and dump

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

rte_mempool_create_empty() adds a mempool to the mempool list before
the mempool has a store. The store is allocated, and
RTE_MEMPOOL_F_POOL_CREATED is set, by the first populate call. In
between, rte_mempool_walk() passes the mempool to its callback, and
rte_mempool_list_dump() calls rte_mempool_dump() on it, which reads
the number of objects in the store. With the ring driver this
dereferences a NULL ring, and the process crashes.

Skip mempools that do not have RTE_MEMPOOL_F_POOL_CREATED set in
rte_mempool_walk() and rte_mempool_list_dump(), and document it. The
populating thread sets the flag without holding a lock, so add a
release fence before setting it and an acquire fence after checking
it. A walker that sees the flag then also sees the store.

rte_mempool_lookup() is unchanged. It does not access the store, and
callers may look up a mempool that is not populated yet.

Fixes: 449c49b93a6b ("mempool: support handler operations")
Cc: stable@dpdk.org

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 lib/mempool/rte_mempool.c | 20 +++++++++++++++++++-
 lib/mempool/rte_mempool.h |  3 +++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index d9f339a3c2..7c4d4697c7 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -304,6 +304,12 @@ mempool_ops_alloc_once(struct rte_mempool *mp)
 		ret = rte_mempool_ops_alloc(mp);
 		if (ret != 0)
 			return ret;
+		/*
+		 * rte_mempool_walk() and rte_mempool_list_dump() access the
+		 * store of pools with this flag set. Make the store visible
+		 * before the flag.
+		 */
+		rte_atomic_thread_fence(rte_memory_order_release);
 		mp->flags |= RTE_MEMPOOL_F_POOL_CREATED;
 	}
 	return 0;
@@ -1412,6 +1418,11 @@ rte_mempool_list_dump(FILE *f)
 
 	TAILQ_FOREACH(te, mempool_list, next) {
 		mp = (struct rte_mempool *) te->data;
+		/* skip mempools that are not populated yet */
+		if ((mp->flags & RTE_MEMPOOL_F_POOL_CREATED) == 0)
+			continue;
+		/* pairs with the release fence in mempool_ops_alloc_once() */
+		rte_atomic_thread_fence(rte_memory_order_acquire);
 		rte_mempool_dump(f, mp);
 	}
 
@@ -1460,7 +1471,14 @@ void rte_mempool_walk(void (*func)(struct rte_mempool *, void *),
 	rte_mcfg_mempool_read_lock();
 
 	RTE_TAILQ_FOREACH_SAFE(te, mempool_list, next, tmp_te) {
-		(*func)((struct rte_mempool *) te->data, arg);
+		struct rte_mempool *mp = (struct rte_mempool *) te->data;
+
+		/* skip mempools that are not populated yet */
+		if ((mp->flags & RTE_MEMPOOL_F_POOL_CREATED) == 0)
+			continue;
+		/* pairs with the release fence in mempool_ops_alloc_once() */
+		rte_atomic_thread_fence(rte_memory_order_acquire);
+		(*func)(mp, arg);
 	}
 
 	rte_mcfg_mempool_read_unlock();
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index bd58d8a9d5..7782768a2d 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1907,6 +1907,8 @@ static inline void *rte_mempool_get_priv(struct rte_mempool *mp)
 /**
  * Dump the status of all mempools on the console
  *
+ * Mempools that are not populated yet are skipped.
+ *
  * @param f
  *   A pointer to a file for output
  */
@@ -1949,6 +1951,7 @@ uint32_t rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
  *
  * The mempool list is locked while the iterator function runs, so the
  * iterator function must not create or free a mempool.
+ * Mempools that are not populated yet are skipped.
  *
  * @param func
  *   Iterator function
-- 
2.34.1

[PATCH v1 3/3] test/mempool: add list walk race test

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

Walk, look up and dump the mempool list on some lcores while other
lcores create and free mempools, in two phases of 500 ms. In the
first phase, mempools are created and freed as fast as possible. In
the second phase, every other mempool is created empty and populated
after a short delay, so that the walkers also find mempools that are
not populated yet.

Without either of the two previous patches, the test crashes. It
needs at least 3 lcores and is skipped otherwise. It adds about one
second to mempool_autotest.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 app/test/test_mempool.c | 137 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 137 insertions(+)

diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c
index 0a4d794346..ae19064cb1 100644
--- a/app/test/test_mempool.c
+++ b/app/test/test_mempool.c
@@ -839,6 +839,139 @@ test_mempool_flag_non_io_unset_when_populated_with_valid_iova(void)
 
 #pragma pop_macro("RTE_TEST_TRACE_FAILURE")
 
+/*
+ * Walk, look up and dump the mempool list on some lcores while other lcores
+ * create and free mempools. In the first phase the creators free mempools as
+ * fast as possible. In the second phase every other mempool is created empty
+ * and populated after a short delay, so that the walkers also find mempools
+ * that are not populated yet.
+ */
+#define LIST_RACE_PHASE_MS	500
+#define LIST_RACE_NB_OBJ	63
+#define LIST_RACE_ELT_SIZE	64
+
+static RTE_ATOMIC(uint32_t) list_race_stop;
+static int list_race_populate_later;
+static FILE *list_race_dump;
+
+static int
+test_mempool_list_race_create(__rte_unused void *arg)
+{
+	char name[RTE_MEMPOOL_NAMESIZE];
+	struct rte_mempool *mp;
+	int populate_later = 0;
+
+	snprintf(name, sizeof(name), "test_list_race_%u", rte_lcore_id());
+	while (rte_atomic_load_explicit(&list_race_stop,
+			rte_memory_order_relaxed) == 0) {
+		if (populate_later) {
+			mp = rte_mempool_create_empty(name, LIST_RACE_NB_OBJ,
+				LIST_RACE_ELT_SIZE, 0, 0, SOCKET_ID_ANY, 0);
+			if (mp == NULL)
+				RET_ERR();
+			rte_delay_us(10);
+			if (rte_mempool_set_ops_byname(mp,
+					rte_mbuf_best_mempool_ops(), NULL) < 0 ||
+					rte_mempool_populate_default(mp) < 0) {
+				rte_mempool_free(mp);
+				RET_ERR();
+			}
+		} else {
+			mp = rte_mempool_create(name, LIST_RACE_NB_OBJ,
+				LIST_RACE_ELT_SIZE, 0, 0, NULL, NULL, NULL, NULL,
+				SOCKET_ID_ANY, 0);
+			if (mp == NULL)
+				RET_ERR();
+		}
+		rte_mempool_free(mp);
+		if (list_race_populate_later)
+			populate_later = !populate_later;
+	}
+	return 0;
+}
+
+static void
+test_mempool_list_race_walk_cb(struct rte_mempool *mp, void *arg)
+{
+	unsigned int *count = arg;
+
+	/* access both the mempool and its store */
+	*count += mp->size + rte_mempool_avail_count(mp);
+}
+
+static int
+test_mempool_list_race_walk(__rte_unused void *arg)
+{
+	unsigned int count = 0;
+
+	while (rte_atomic_load_explicit(&list_race_stop,
+			rte_memory_order_relaxed) == 0) {
+		rte_mempool_walk(test_mempool_list_race_walk_cb, &count);
+		if (rte_mempool_lookup("test_list_race_none") != NULL)
+			RET_ERR();
+		if (list_race_dump != NULL)
+			rte_mempool_list_dump(list_race_dump);
+	}
+	return 0;
+}
+
+static int
+test_mempool_list_race_phase(int populate_later, FILE *dump)
+{
+	unsigned int nb_creators = (rte_lcore_count() - 1) / 2;
+	unsigned int lcore_id, i = 0;
+	int ret = 0;
+
+	list_race_populate_later = populate_later;
+	list_race_dump = dump;
+	rte_atomic_store_explicit(&list_race_stop, 0, rte_memory_order_relaxed);
+	RTE_LCORE_FOREACH_WORKER(lcore_id) {
+		lcore_function_t *func = i++ < nb_creators ?
+			test_mempool_list_race_create :
+			test_mempool_list_race_walk;
+
+		if (rte_eal_remote_launch(func, NULL, lcore_id) != 0)
+			ret = -1;
+	}
+
+	rte_delay_ms(LIST_RACE_PHASE_MS);
+	rte_atomic_store_explicit(&list_race_stop, 1, rte_memory_order_relaxed);
+
+	RTE_LCORE_FOREACH_WORKER(lcore_id) {
+		if (rte_eal_wait_lcore(lcore_id) < 0)
+			ret = -1;
+	}
+	return ret;
+}
+
+static int
+test_mempool_list_race(void)
+{
+	FILE *f;
+	int ret;
+
+	if (rte_lcore_count() < 3) {
+		printf("%s skipped: needs at least 3 lcores\n", __func__);
+		return 0;
+	}
+
+	/* mempools freed while the list is walked */
+	ret = test_mempool_list_race_phase(0, NULL);
+	if (ret != 0) {
+		LOG_ERR();
+		return ret;
+	}
+
+	/* mempools not populated yet while the list is walked and dumped */
+	f = fopen("/dev/null", "w");
+	ret = test_mempool_list_race_phase(1, f);
+	if (f != NULL)
+		fclose(f);
+	if (ret != 0)
+		LOG_ERR();
+	return ret;
+}
+
 static int
 test_mempool(void)
 {
@@ -1105,6 +1238,10 @@ test_mempool(void)
 	if (test_mempool_flag_non_io_unset_when_populated_with_valid_iova() < 0)
 		GOTO_ERR(ret, err);
 
+	/* test walking the mempool list while mempools are created and freed */
+	if (test_mempool_list_race() < 0)
+		GOTO_ERR(ret, err);
+
 	rte_mempool_list_dump(stdout);
 
 	ret = 0;
-- 
2.34.1

RE: [PATCH v1 1/3] mempool: fix race between list walk and free

[Morten Brørup]

> From: Robert Sanford [mailto:rsanford2@gmail.com]
> Sent: Tuesday, 3 November 2026 02.11
> To: dev@dpdk.org
> Cc: andrew.rybchenko@oktetlabs.ru; Morten Brørup;
> olivier.matz@6wind.com; Robert Sanford; stable@dpdk.org
> Subject: [PATCH v1 1/3] mempool: fix race between list walk and free
> 
> From: Robert Sanford <rsanford@akamai.com>
> 
> rte_mempool_walk(), rte_mempool_lookup() and rte_mempool_list_dump()
> traverse the mempool list holding only the mempool list lock.
> rte_mempool_create_empty() inserts into the list holding both the
> mempool list lock and the tailq lock, but rte_mempool_free() unlinks
> and frees the list entry holding only the tailq lock. So a mempool can
> be unlinked and freed, together with its list entry, while another
> thread is walking the list. The walker then reads freed memory. Since
> freed heap memory is zeroed, the walk callback is passed a NULL
> mempool, or rte_mempool_lookup() compares the name of one, and the
> process crashes.
> 
> Take the mempool list lock in rte_mempool_free() as well, in the same
> order as rte_mempool_create_empty(). As a result, a rte_mempool_walk()
> callback must not free a mempool, as it already could not create one.
> Document this.
> 
> Fixes: a3791ff0a3f8 ("mempool: introduce a function to free a pool")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Robert Sanford <rsanford@akamai.com>
> ---
>  lib/mempool/rte_mempool.c | 8 ++++++++
>  lib/mempool/rte_mempool.h | 3 +++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index 069aabaa70..d9f339a3c2 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -729,6 +729,13 @@ rte_mempool_free(struct rte_mempool *mp)
>  		return;
> 
>  	mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head,
> rte_mempool_list);
> +	/*
> +	 * Take the mempool list lock as well, like
> rte_mempool_create_empty(),
> +	 * so that the entry is not unlinked and freed while
> +	 * rte_mempool_walk(), rte_mempool_lookup() or
> rte_mempool_list_dump()
> +	 * is traversing the list.
> +	 */
> +	rte_mcfg_mempool_write_lock();
>  	rte_mcfg_tailq_write_lock();
>  	/* find out tailq entry */
>  	TAILQ_FOREACH(te, mempool_list, next) {
> @@ -741,6 +748,7 @@ rte_mempool_free(struct rte_mempool *mp)
>  		rte_free(te);
>  	}
>  	rte_mcfg_tailq_write_unlock();
> +	rte_mcfg_mempool_write_unlock();
> 
>  	mempool_event_callback_invoke(RTE_MEMPOOL_EVENT_DESTROY, mp);
>  	rte_mempool_trace_free(mp);
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index b34cd893b1..bd58d8a9d5 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -1947,6 +1947,9 @@ uint32_t rte_mempool_calc_obj_size(uint32_t
> elt_size, uint32_t flags,
>  /**
>   * Walk list of all memory pools
>   *
> + * The mempool list is locked while the iterator function runs, so the
> + * iterator function must not create or free a mempool.
> + *
>   * @param func
>   *   Iterator function
>   * @param arg
> --
> 2.34.1

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Verified that rte_mempool_create_empty() takes the mcfg_mempool and mcfg_tailq locks in the same order as this patch.

RE: [PATCH v1 2/3] mempool: skip unpopulated pools in walk and dump

[Morten Brørup]

> From: Robert Sanford <rsanford@akamai.com>
> 
> rte_mempool_create_empty() adds a mempool to the mempool list before
> the mempool has a store. The store is allocated, and
> RTE_MEMPOOL_F_POOL_CREATED is set, by the first populate call. In
> between, rte_mempool_walk() passes the mempool to its callback, and
> rte_mempool_list_dump() calls rte_mempool_dump() on it, which reads
> the number of objects in the store. With the ring driver this
> dereferences a NULL ring, and the process crashes.
> 
> Skip mempools that do not have RTE_MEMPOOL_F_POOL_CREATED set in
> rte_mempool_walk() and rte_mempool_list_dump(), and document it.

NAK to rte_mempool_walk() skipping unpopulated pools. The effect is too broad.

The correct solution is for the relevant callback functions to check RTE_MEMPOOL_F_POOL_CREATED, if it needs to access the mempool driver.

Instead, please fix rte_mempool_dump(FILE *f, struct rte_mempool *mp) to not dump the parts accessing the mempool driver when RTE_MEMPOOL_F_POOL_CREATED is not set. We want to know that the mempool exists, and partial information is much better than no information.

> The
> populating thread sets the flag without holding a lock, so add a
> release fence before setting it and an acquire fence after checking
> it. A walker that sees the flag then also sees the store.
> 
> rte_mempool_lookup() is unchanged. It does not access the store, and
> callers may look up a mempool that is not populated yet.
> 
> Fixes: 449c49b93a6b ("mempool: support handler operations")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Robert Sanford <rsanford@akamai.com>
> ---
>  lib/mempool/rte_mempool.c | 20 +++++++++++++++++++-
>  lib/mempool/rte_mempool.h |  3 +++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index d9f339a3c2..7c4d4697c7 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -304,6 +304,12 @@ mempool_ops_alloc_once(struct rte_mempool *mp)
>  		ret = rte_mempool_ops_alloc(mp);
>  		if (ret != 0)
>  			return ret;
> +		/*
> +		 * rte_mempool_walk() and rte_mempool_list_dump() access
> the
> +		 * store of pools with this flag set. Make the store
> visible
> +		 * before the flag.
> +		 */
> +		rte_atomic_thread_fence(rte_memory_order_release);
>  		mp->flags |= RTE_MEMPOOL_F_POOL_CREATED;
>  	}
>  	return 0;
> @@ -1412,6 +1418,11 @@ rte_mempool_list_dump(FILE *f)
> 
>  	TAILQ_FOREACH(te, mempool_list, next) {
>  		mp = (struct rte_mempool *) te->data;
> +		/* skip mempools that are not populated yet */
> +		if ((mp->flags & RTE_MEMPOOL_F_POOL_CREATED) == 0)
> +			continue;
> +		/* pairs with the release fence in mempool_ops_alloc_once()
> */
> +		rte_atomic_thread_fence(rte_memory_order_acquire);
>  		rte_mempool_dump(f, mp);
>  	}
> 
> @@ -1460,7 +1471,14 @@ void rte_mempool_walk(void (*func)(struct
> rte_mempool *, void *),
>  	rte_mcfg_mempool_read_lock();
> 
>  	RTE_TAILQ_FOREACH_SAFE(te, mempool_list, next, tmp_te) {
> -		(*func)((struct rte_mempool *) te->data, arg);
> +		struct rte_mempool *mp = (struct rte_mempool *) te->data;
> +
> +		/* skip mempools that are not populated yet */
> +		if ((mp->flags & RTE_MEMPOOL_F_POOL_CREATED) == 0)
> +			continue;
> +		/* pairs with the release fence in mempool_ops_alloc_once()
> */
> +		rte_atomic_thread_fence(rte_memory_order_acquire);
> +		(*func)(mp, arg);
>  	}
> 
>  	rte_mcfg_mempool_read_unlock();
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index bd58d8a9d5..7782768a2d 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -1907,6 +1907,8 @@ static inline void *rte_mempool_get_priv(struct
> rte_mempool *mp)
>  /**
>   * Dump the status of all mempools on the console
>   *
> + * Mempools that are not populated yet are skipped.
> + *
>   * @param f
>   *   A pointer to a file for output
>   */
> @@ -1949,6 +1951,7 @@ uint32_t rte_mempool_calc_obj_size(uint32_t
> elt_size, uint32_t flags,
>   *
>   * The mempool list is locked while the iterator function runs, so the
>   * iterator function must not create or free a mempool.
> + * Mempools that are not populated yet are skipped.
>   *
>   * @param func
>   *   Iterator function
> --
> 2.34.1

RE: [PATCH v1 3/3] test/mempool: add list walk race test

[Morten Brørup]

> From: Robert Sanford <rsanford@akamai.com>
> 
> Walk, look up and dump the mempool list on some lcores while other
> lcores create and free mempools, in two phases of 500 ms. In the
> first phase, mempools are created and freed as fast as possible. In
> the second phase, every other mempool is created empty and populated
> after a short delay, so that the walkers also find mempools that are
> not populated yet.
> 
> Without either of the two previous patches, the test crashes. It
> needs at least 3 lcores and is skipped otherwise. It adds about one
> second to mempool_autotest.
> 
> Signed-off-by: Robert Sanford <rsanford@akamai.com>
> ---

This patch 3/3 might need some changes when patch 2/3 has been changed.
Holding back on reviewing until v2 of the series.

-Morten

[PATCH v2 0/3] mempool: fix races between list walk and create/free

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

A thread walking the mempool list with rte_mempool_walk(),
rte_mempool_lookup() or rte_mempool_list_dump() can crash while other
threads create and free mempools. We hit this in production, in a
process that walks the mempools while other threads create and free
them.

There are two races:

- rte_mempool_free() unlinks and frees the list entry without the
  mempool list lock that the walkers hold, so a walker can reach a
  freed entry or mempool. Patch 1 takes the lock in rte_mempool_free(),
  as rte_mempool_create_empty() already does. A rte_mempool_walk()
  callback can therefore no longer free a mempool; this is documented.

- A mempool is on the list before its store exists, but
  rte_mempool_dump() queries the mempool driver, which reads the store.
  So rte_mempool_list_dump() and the telemetry /mempool/info handler
  crash on such a mempool. Patch 2 makes them report the mempool
  without querying the driver until RTE_MEMPOOL_F_POOL_CREATED is set,
  and documents that rte_mempool_walk() callbacks must check that flag
  before querying the driver.

Patch 3 adds a test to mempool_autotest. On x86 with 4 or 8 lcores,
it crashed in all 20 runs without patch 1, without patch 2, or
without both, and passed in all 20 runs with both patches.

v2:
- Patch 2 reworked as suggested by Morten Brørup: rte_mempool_walk()
  no longer skips mempools that are not populated yet. Instead,
  rte_mempool_dump() and the telemetry callback report them without
  querying the mempool driver, and the rte_mempool_walk() documentation
  asks callbacks to check RTE_MEMPOOL_F_POOL_CREATED. The flag is now
  set and tested with release/acquire atomics instead of plain accesses
  with fences. New title.
- Patch 3: the walk callback dumps every mempool, and checks the flag
  before querying the driver. The test waits 100 us instead of 10 us
  before populating, so that it still finds unpopulated mempools
  reliably.
- Patch 1: unchanged, Reviewed-by added.

Robert Sanford (3):
  mempool: fix race between list walk and free
  mempool: fix dump of unpopulated pool
  test/mempool: add list walk race test

 app/test/test_mempool.c   | 146 ++++++++++++++++++++++++++++++++++++++
 lib/mempool/rte_mempool.c |  44 ++++++++++--
 lib/mempool/rte_mempool.h |  12 ++++
 3 files changed, 198 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] mempool: fix race between list walk and free

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

rte_mempool_walk(), rte_mempool_lookup() and rte_mempool_list_dump()
traverse the mempool list holding only the mempool list lock.
rte_mempool_create_empty() inserts into the list holding both the
mempool list lock and the tailq lock, but rte_mempool_free() unlinks
and frees the list entry holding only the tailq lock. So a mempool can
be unlinked and freed, together with its list entry, while another
thread is walking the list. The walker then reads freed memory. Since
freed heap memory is zeroed, the walk callback is passed a NULL
mempool, or rte_mempool_lookup() compares the name of one, and the
process crashes.

Take the mempool list lock in rte_mempool_free() as well, in the same
order as rte_mempool_create_empty(). As a result, a rte_mempool_walk()
callback must not free a mempool, as it already could not create one.
Document this.

Fixes: a3791ff0a3f8 ("mempool: introduce a function to free a pool")
Cc: stable@dpdk.org

Signed-off-by: Robert Sanford <rsanford@akamai.com>
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/mempool/rte_mempool.c | 8 ++++++++
 lib/mempool/rte_mempool.h | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index 069aabaa70..d9f339a3c2 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -729,6 +729,13 @@ rte_mempool_free(struct rte_mempool *mp)
 		return;
 
 	mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempool_list);
+	/*
+	 * Take the mempool list lock as well, like rte_mempool_create_empty(),
+	 * so that the entry is not unlinked and freed while
+	 * rte_mempool_walk(), rte_mempool_lookup() or rte_mempool_list_dump()
+	 * is traversing the list.
+	 */
+	rte_mcfg_mempool_write_lock();
 	rte_mcfg_tailq_write_lock();
 	/* find out tailq entry */
 	TAILQ_FOREACH(te, mempool_list, next) {
@@ -741,6 +748,7 @@ rte_mempool_free(struct rte_mempool *mp)
 		rte_free(te);
 	}
 	rte_mcfg_tailq_write_unlock();
+	rte_mcfg_mempool_write_unlock();
 
 	mempool_event_callback_invoke(RTE_MEMPOOL_EVENT_DESTROY, mp);
 	rte_mempool_trace_free(mp);
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index b34cd893b1..bd58d8a9d5 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1947,6 +1947,9 @@ uint32_t rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
 /**
  * Walk list of all memory pools
  *
+ * The mempool list is locked while the iterator function runs, so the
+ * iterator function must not create or free a mempool.
+ *
  * @param func
  *   Iterator function
  * @param arg
-- 
2.34.1

[PATCH v2 2/3] mempool: fix dump of unpopulated pool

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

rte_mempool_create_empty() adds a mempool to the mempool list before
the mempool has a store. The store is allocated, and
RTE_MEMPOOL_F_POOL_CREATED is set, by the first populate call. In
between, rte_mempool_dump() queries the mempool driver for the number
of objects in the store. With the ring driver this dereferences a NULL
ring, and the process crashes. rte_mempool_list_dump() calls
rte_mempool_dump() for every mempool on the list, so it crashes when
another thread has created a mempool but not populated it yet. The
telemetry /mempool/info handler has the same problem: its
rte_mempool_walk() callback queries the driver for the same count.

In rte_mempool_dump() and in the telemetry callback, query the mempool
driver only when RTE_MEMPOOL_F_POOL_CREATED is set, and report a common
pool count of 0 otherwise, since the mempool has no objects yet. The
rest of the mempool information is still reported. In statistics
builds, rte_mempool_dump() also skips rte_mempool_ops_get_info() for
such a mempool. The info is now zeroed first; it was left uninitialized
for drivers without a get_info operation.

The populating thread sets the flag without holding a lock, so set it
with an atomic fetch-or with release ordering, and test it with an
atomic load with acquire ordering. A reader that sees the flag then
also sees the store.

rte_mempool_walk() still passes mempools that are not populated yet to
its callback. Document that the callback must check
RTE_MEMPOOL_F_POOL_CREATED before querying the mempool driver.

Fixes: 449c49b93a6b ("mempool: support handler operations")
Fixes: ab8a1cc4e3d8 ("mempool: add object usage counts in telemetry")
Cc: stable@dpdk.org

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 lib/mempool/rte_mempool.c | 36 ++++++++++++++++++++++++++++++++----
 lib/mempool/rte_mempool.h |  9 +++++++++
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index d9f339a3c2..c9da42441a 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -294,6 +294,21 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
 	}
 }
 
+/*
+ * Check whether the mempool driver has allocated its store. Mempools are on
+ * the mempool list before that, so rte_mempool_dump() and rte_mempool_walk()
+ * callbacks can find them while another thread populates them.
+ * Acquire: pairs with the release in mempool_ops_alloc_once().
+ */
+static bool
+mempool_is_created(const struct rte_mempool *mp)
+{
+	return (rte_atomic_load_explicit(
+			(const RTE_ATOMIC(unsigned int) *)&mp->flags,
+			rte_memory_order_acquire) &
+		RTE_MEMPOOL_F_POOL_CREATED) != 0;
+}
+
 static int
 mempool_ops_alloc_once(struct rte_mempool *mp)
 {
@@ -304,7 +319,13 @@ mempool_ops_alloc_once(struct rte_mempool *mp)
 		ret = rte_mempool_ops_alloc(mp);
 		if (ret != 0)
 			return ret;
-		mp->flags |= RTE_MEMPOOL_F_POOL_CREATED;
+		/*
+		 * Release: make the store visible before the flag, see
+		 * mempool_is_created().
+		 */
+		rte_atomic_fetch_or_explicit(
+			(RTE_ATOMIC(unsigned int) *)&mp->flags,
+			RTE_MEMPOOL_F_POOL_CREATED, rte_memory_order_release);
 	}
 	return 0;
 }
@@ -1283,10 +1304,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	unsigned common_count;
 	unsigned cache_count;
 	size_t mem_len = 0;
+	bool created;
 
 	RTE_ASSERT(f != NULL);
 	RTE_ASSERT(mp != NULL);
 
+	/* the mempool driver can only be queried once the store exists */
+	created = mempool_is_created(mp);
+
 	fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
 	fprintf(f, "  flags=%x\n", mp->flags);
 	fprintf(f, "  socket_id=%d\n", mp->socket_id);
@@ -1318,14 +1343,16 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	}
 
 	cache_count = rte_mempool_dump_cache(f, mp);
-	common_count = rte_mempool_ops_get_count(mp);
+	common_count = created ? rte_mempool_ops_get_count(mp) : 0;
 	if ((cache_count + common_count) > mp->size)
 		common_count = mp->size - cache_count;
 	fprintf(f, "  common_pool_count=%u\n", common_count);
 
 	/* sum and dump statistics */
 #ifdef RTE_LIBRTE_MEMPOOL_STATS
-	rte_mempool_ops_get_info(mp, &info);
+	memset(&info, 0, sizeof(info));
+	if (created)
+		rte_mempool_ops_get_info(mp, &info);
 	memset(&sum, 0, sizeof(sum));
 	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE + 1; lcore_id++) {
 		sum.put_bulk += mp->stats[lcore_id].put_bulk;
@@ -1653,7 +1680,8 @@ mempool_info_cb(struct rte_mempool *mp, void *arg)
 			cache_count += cache->len;
 	}
 	rte_tel_data_add_dict_uint(info->d, "total_cache_count", cache_count);
-	common_count = rte_mempool_ops_get_count(mp);
+	/* the mempool driver can only be queried once the store exists */
+	common_count = mempool_is_created(mp) ? rte_mempool_ops_get_count(mp) : 0;
 	if ((cache_count + common_count) > mp->size)
 		common_count = mp->size - cache_count;
 	rte_tel_data_add_dict_uint(info->d, "common_pool_count", common_count);
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index bd58d8a9d5..d8413aa4bf 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1325,6 +1325,9 @@ void rte_mempool_stats_reset(struct rte_mempool *mp);
 /**
  * Dump the status of the mempool to a file.
  *
+ * For a mempool that is not populated yet, the mempool driver is not
+ * queried, and the common pool count is reported as 0.
+ *
  * @param f
  *   A pointer to a file for output
  * @param mp
@@ -1950,6 +1953,12 @@ uint32_t rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
  * The mempool list is locked while the iterator function runs, so the
  * iterator function must not create or free a mempool.
  *
+ * The iterator function is also called for mempools that are not
+ * populated yet, which do not have RTE_MEMPOOL_F_POOL_CREATED set in
+ * their flags. Such a mempool has no store yet, so the iterator function
+ * must check this flag before calling functions that query the mempool
+ * driver, such as rte_mempool_avail_count().
+ *
  * @param func
  *   Iterator function
  * @param arg
-- 
2.34.1

[PATCH v2 3/3] test/mempool: add list walk race test

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

Walk, look up and dump the mempool list on some lcores while other
lcores create and free mempools, in two phases of 500 ms. In the
first phase, mempools are created and freed as fast as possible. In
the second phase, every other mempool is created empty and populated
after a short delay, so that the walkers also find mempools that are
not populated yet. In that phase, the walk callback also dumps each
mempool, and it checks RTE_MEMPOOL_F_POOL_CREATED before querying the
mempool driver, as documented for rte_mempool_walk().

Without either of the two previous patches, the test crashes. It
needs at least 3 lcores and is skipped otherwise. It adds about one
second to mempool_autotest.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 app/test/test_mempool.c | 146 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 146 insertions(+)

diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c
index 0a4d794346..a8daa813c7 100644
--- a/app/test/test_mempool.c
+++ b/app/test/test_mempool.c
@@ -839,6 +839,148 @@ test_mempool_flag_non_io_unset_when_populated_with_valid_iova(void)
 
 #pragma pop_macro("RTE_TEST_TRACE_FAILURE")
 
+/*
+ * Walk, look up and dump the mempool list on some lcores while other lcores
+ * create and free mempools. In the first phase the creators free mempools as
+ * fast as possible. In the second phase every other mempool is created empty
+ * and populated after a short delay, so that the walkers also find mempools
+ * that are not populated yet, and dump them.
+ */
+#define LIST_RACE_PHASE_MS	500
+#define LIST_RACE_NB_OBJ	63
+#define LIST_RACE_ELT_SIZE	64
+
+static RTE_ATOMIC(uint32_t) list_race_stop;
+static int list_race_populate_later;
+static FILE *list_race_dump;
+
+static int
+test_mempool_list_race_create(__rte_unused void *arg)
+{
+	char name[RTE_MEMPOOL_NAMESIZE];
+	struct rte_mempool *mp;
+	int populate_later = 0;
+
+	snprintf(name, sizeof(name), "test_list_race_%u", rte_lcore_id());
+	while (rte_atomic_load_explicit(&list_race_stop,
+			rte_memory_order_relaxed) == 0) {
+		if (populate_later) {
+			mp = rte_mempool_create_empty(name, LIST_RACE_NB_OBJ,
+				LIST_RACE_ELT_SIZE, 0, 0, SOCKET_ID_ANY, 0);
+			if (mp == NULL)
+				RET_ERR();
+			rte_delay_us(100);
+			if (rte_mempool_set_ops_byname(mp,
+					rte_mbuf_best_mempool_ops(), NULL) < 0 ||
+					rte_mempool_populate_default(mp) < 0) {
+				rte_mempool_free(mp);
+				RET_ERR();
+			}
+		} else {
+			mp = rte_mempool_create(name, LIST_RACE_NB_OBJ,
+				LIST_RACE_ELT_SIZE, 0, 0, NULL, NULL, NULL, NULL,
+				SOCKET_ID_ANY, 0);
+			if (mp == NULL)
+				RET_ERR();
+		}
+		rte_mempool_free(mp);
+		if (list_race_populate_later)
+			populate_later = !populate_later;
+	}
+	return 0;
+}
+
+static void
+test_mempool_list_race_walk_cb(struct rte_mempool *mp, void *arg)
+{
+	unsigned int *count = arg;
+
+	/* rte_mempool_dump() must handle mempools that are not populated yet */
+	if (list_race_dump != NULL)
+		rte_mempool_dump(list_race_dump, mp);
+
+	/* as documented for rte_mempool_walk(), check the flag first */
+	if ((rte_atomic_load_explicit((RTE_ATOMIC(unsigned int) *)&mp->flags,
+			rte_memory_order_acquire) &
+			RTE_MEMPOOL_F_POOL_CREATED) == 0)
+		return;
+	/* access the store */
+	*count += mp->size + rte_mempool_avail_count(mp);
+}
+
+static int
+test_mempool_list_race_walk(__rte_unused void *arg)
+{
+	unsigned int count = 0;
+
+	while (rte_atomic_load_explicit(&list_race_stop,
+			rte_memory_order_relaxed) == 0) {
+		rte_mempool_walk(test_mempool_list_race_walk_cb, &count);
+		if (rte_mempool_lookup("test_list_race_none") != NULL)
+			RET_ERR();
+		if (list_race_dump != NULL)
+			rte_mempool_list_dump(list_race_dump);
+	}
+	return 0;
+}
+
+static int
+test_mempool_list_race_phase(int populate_later, FILE *dump)
+{
+	unsigned int nb_creators = (rte_lcore_count() - 1) / 2;
+	unsigned int lcore_id, i = 0;
+	int ret = 0;
+
+	list_race_populate_later = populate_later;
+	list_race_dump = dump;
+	rte_atomic_store_explicit(&list_race_stop, 0, rte_memory_order_relaxed);
+	RTE_LCORE_FOREACH_WORKER(lcore_id) {
+		lcore_function_t *func = i++ < nb_creators ?
+			test_mempool_list_race_create :
+			test_mempool_list_race_walk;
+
+		if (rte_eal_remote_launch(func, NULL, lcore_id) != 0)
+			ret = -1;
+	}
+
+	rte_delay_ms(LIST_RACE_PHASE_MS);
+	rte_atomic_store_explicit(&list_race_stop, 1, rte_memory_order_relaxed);
+
+	RTE_LCORE_FOREACH_WORKER(lcore_id) {
+		if (rte_eal_wait_lcore(lcore_id) < 0)
+			ret = -1;
+	}
+	return ret;
+}
+
+static int
+test_mempool_list_race(void)
+{
+	FILE *f;
+	int ret;
+
+	if (rte_lcore_count() < 3) {
+		printf("%s skipped: needs at least 3 lcores\n", __func__);
+		return 0;
+	}
+
+	/* mempools freed while the list is walked */
+	ret = test_mempool_list_race_phase(0, NULL);
+	if (ret != 0) {
+		LOG_ERR();
+		return ret;
+	}
+
+	/* mempools not populated yet while the list is walked and dumped */
+	f = fopen("/dev/null", "w");
+	ret = test_mempool_list_race_phase(1, f);
+	if (f != NULL)
+		fclose(f);
+	if (ret != 0)
+		LOG_ERR();
+	return ret;
+}
+
 static int
 test_mempool(void)
 {
@@ -1105,6 +1247,10 @@ test_mempool(void)
 	if (test_mempool_flag_non_io_unset_when_populated_with_valid_iova() < 0)
 		GOTO_ERR(ret, err);
 
+	/* test walking the mempool list while mempools are created and freed */
+	if (test_mempool_list_race() < 0)
+		GOTO_ERR(ret, err);
+
 	rte_mempool_list_dump(stdout);
 
 	ret = 0;
-- 
2.34.1

RE: [PATCH v2 2/3] mempool: fix dump of unpopulated pool

[Morten Brørup]

Good progress, Robert.

One more change requested, see comments inline below.

> +/*
> + * Check whether the mempool driver has allocated its store. Mempools
> are on
> + * the mempool list before that, so rte_mempool_dump() and
> rte_mempool_walk()
> + * callbacks can find them while another thread populates them.
> + * Acquire: pairs with the release in mempool_ops_alloc_once().
> + */
> +static bool
> +mempool_is_created(const struct rte_mempool *mp)
> +{
> +	return (rte_atomic_load_explicit(
> +			(const RTE_ATOMIC(unsigned int) *)&mp->flags,
> +			rte_memory_order_acquire) &
> +		RTE_MEMPOOL_F_POOL_CREATED) != 0;
> +}

mempool_is_created() should be public as rte_mempool_is_created().
Then the rte_mempool_walk() description can refer to it instead of referring to the RTE_MEMPOOL_F_POOL_CREATED flag.

The "Acquire:" comment is an implementation detail; when you make the function public, move that comment away from the function description and into the implementation instead.

About the function name:
When the empty mempool is created, it is assigned the "ring" driver as default.
Until the pool has been populated, it may be assigned a different mempool driver, e.g. the "stack" driver.
I suppose this is why the flag is named CREATED and not POPULATED.
And thus, the name rte_mempool_is_created() remains correct, rather than using a different name.
You may consider this information, if you choose to update the function description.

Notes:
Although the function is short and simple, it is not fast path, so it should remain non-inline.
You already made it non-inline, so I'm just making sure you keep it that way. :-)

It is used by mempool dump, so I don't think it can be marked experimental.

[...]

>   * The mempool list is locked while the iterator function runs, so the
>   * iterator function must not create or free a mempool.
>   *
> + * The iterator function is also called for mempools that are not
> + * populated yet, which do not have RTE_MEMPOOL_F_POOL_CREATED set in
> + * their flags. Such a mempool has no store yet, so the iterator
> function
> + * must check this flag before calling functions that query the

Remember referring to the new rte_mempool_is_created() function in this description.

> mempool
> + * driver, such as rte_mempool_avail_count().
> + *
>   * @param func
>   *   Iterator function
>   * @param arg

With the suggested change,
Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v2 2/3] mempool: fix dump of unpopulated pool

[Robert Sanford]

Hi Morten,

Thanks for the review.

> mempool_is_created() should be public as rte_mempool_is_created().

I made the change: rte_mempool_is_created() is public, non-inline,
not experimental, and the rte_mempool_walk() description refers to it.

However, devtools/check-symbol-change.py reports:

  ERROR: rte_mempool_is_created in lib/mempool has been added directly
  to stable ABI.

How would you like me to proceed?

Thanks,
Robert

RE: [PATCH v2 2/3] mempool: fix dump of unpopulated pool

[Morten Brørup]

> From: Robert Sanford [mailto:rsanford2@gmail.com]
> Sent: Friday, 6 November 2026 19.43
> 
> Hi Morten,
> 
> Thanks for the review.
> 
> > mempool_is_created() should be public as rte_mempool_is_created().
> 
> I made the change: rte_mempool_is_created() is public, non-inline,
> not experimental, and the rte_mempool_walk() description refers to it.
> 
> However, devtools/check-symbol-change.py reports:
> 
>   ERROR: rte_mempool_is_created in lib/mempool has been added directly
>   to stable ABI.
> 
> How would you like me to proceed?

Please ignore the error, and send the series anyway.
Patch acceptance is a manual process, and we can ignore errors like this, when relevant.

I consider the alternative, an experimental public function, and a private function doing exactly the same (or copy-pasted code to atomically check the "created" flag), an ugly hack.

-Morten

[PATCH v3 0/3] mempool: fix races between list walk and create/free

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

A thread walking the mempool list with rte_mempool_walk(),
rte_mempool_lookup() or rte_mempool_list_dump() can crash while other
threads create and free mempools. We hit this in production, in a
process that walks the mempools while other threads create and free
them.

There are two races:

- rte_mempool_free() unlinks and frees the list entry without the
  mempool list lock that the walkers hold, so a walker can reach a
  freed entry or mempool. Patch 1 takes the lock in rte_mempool_free(),
  as rte_mempool_create_empty() already does. A rte_mempool_walk()
  callback can therefore no longer free a mempool; this is documented.

- A mempool is on the list before its store exists, but
  rte_mempool_dump() queries the mempool driver, which reads the store.
  So rte_mempool_list_dump() and the telemetry /mempool/info handler
  crash on such a mempool. Patch 2 adds rte_mempool_is_created(),
  makes them report the mempool without querying the driver until it
  returns true, and documents that rte_mempool_walk() callbacks must
  check it before querying the driver.

Patch 3 adds a test to mempool_autotest. On x86 with 4 or 8 lcores,
it crashed in all 20 runs without patch 1, without patch 2, or
without both, and passed in all 20 runs with both patches.

v3:
- Patch 2, as suggested by Morten Brørup: the helper that tests
  RTE_MEMPOOL_F_POOL_CREATED is now the public function
  rte_mempool_is_created(), and the rte_mempool_walk() documentation
  refers to it instead of the flag. Release note added. Acked-by added.
- Patch 3: the walk callback uses rte_mempool_is_created().
- Patch 1: unchanged.

v2:
- Patch 2 reworked as suggested by Morten Brørup: rte_mempool_walk()
  no longer skips mempools that are not populated yet. Instead,
  rte_mempool_dump() and the telemetry callback report them without
  querying the mempool driver, and the rte_mempool_walk() documentation
  asks callbacks to check RTE_MEMPOOL_F_POOL_CREATED. The flag is now
  set and tested with release/acquire atomics instead of plain accesses
  with fences. New title.
- Patch 3: the walk callback dumps every mempool, and checks the flag
  before querying the driver. The test waits 100 us instead of 10 us
  before populating, so that it still finds unpopulated mempools
  reliably.
- Patch 1: unchanged, Reviewed-by added.

Robert Sanford (3):
  mempool: fix race between list walk and free
  mempool: fix dump of unpopulated pool
  test/mempool: add list walk race test

 app/test/test_mempool.c                | 144 +++++++++++++++++++++++++
 doc/guides/rel_notes/release_26_11.rst |   7 ++
 lib/mempool/rte_mempool.c              |  45 +++++++-
 lib/mempool/rte_mempool.h              |  29 +++++
 4 files changed, 221 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v3 1/3] mempool: fix race between list walk and free

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

rte_mempool_walk(), rte_mempool_lookup() and rte_mempool_list_dump()
traverse the mempool list holding only the mempool list lock.
rte_mempool_create_empty() inserts into the list holding both the
mempool list lock and the tailq lock, but rte_mempool_free() unlinks
and frees the list entry holding only the tailq lock. So a mempool can
be unlinked and freed, together with its list entry, while another
thread is walking the list. The walker then reads freed memory. Since
freed heap memory is zeroed, the walk callback is passed a NULL
mempool, or rte_mempool_lookup() compares the name of one, and the
process crashes.

Take the mempool list lock in rte_mempool_free() as well, in the same
order as rte_mempool_create_empty(). As a result, a rte_mempool_walk()
callback must not free a mempool, as it already could not create one.
Document this.

Fixes: a3791ff0a3f8 ("mempool: introduce a function to free a pool")
Cc: stable@dpdk.org

Signed-off-by: Robert Sanford <rsanford@akamai.com>
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/mempool/rte_mempool.c | 8 ++++++++
 lib/mempool/rte_mempool.h | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index 069aabaa70..d9f339a3c2 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -729,6 +729,13 @@ rte_mempool_free(struct rte_mempool *mp)
 		return;
 
 	mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempool_list);
+	/*
+	 * Take the mempool list lock as well, like rte_mempool_create_empty(),
+	 * so that the entry is not unlinked and freed while
+	 * rte_mempool_walk(), rte_mempool_lookup() or rte_mempool_list_dump()
+	 * is traversing the list.
+	 */
+	rte_mcfg_mempool_write_lock();
 	rte_mcfg_tailq_write_lock();
 	/* find out tailq entry */
 	TAILQ_FOREACH(te, mempool_list, next) {
@@ -741,6 +748,7 @@ rte_mempool_free(struct rte_mempool *mp)
 		rte_free(te);
 	}
 	rte_mcfg_tailq_write_unlock();
+	rte_mcfg_mempool_write_unlock();
 
 	mempool_event_callback_invoke(RTE_MEMPOOL_EVENT_DESTROY, mp);
 	rte_mempool_trace_free(mp);
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index b34cd893b1..bd58d8a9d5 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1947,6 +1947,9 @@ uint32_t rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
 /**
  * Walk list of all memory pools
  *
+ * The mempool list is locked while the iterator function runs, so the
+ * iterator function must not create or free a mempool.
+ *
  * @param func
  *   Iterator function
  * @param arg
-- 
2.34.1

[PATCH v3 2/3] mempool: fix dump of unpopulated pool

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

rte_mempool_create_empty() adds a mempool to the mempool list before
the mempool has a store. The store is allocated, and
RTE_MEMPOOL_F_POOL_CREATED is set, by the first populate call. In
between, rte_mempool_dump() queries the mempool driver for the number
of objects in the store. With the ring driver this dereferences a NULL
ring, and the process crashes. rte_mempool_list_dump() calls
rte_mempool_dump() for every mempool on the list, so it crashes when
another thread has created a mempool but not populated it yet. The
telemetry /mempool/info handler has the same problem: its
rte_mempool_walk() callback queries the driver for the same count.

Add rte_mempool_is_created(), which tests RTE_MEMPOOL_F_POOL_CREATED.
In rte_mempool_dump() and in the telemetry callback, query the mempool
driver only when it returns true, and report a common
pool count of 0 otherwise, since the mempool has no objects yet. The
rest of the mempool information is still reported. In statistics
builds, rte_mempool_dump() also skips rte_mempool_ops_get_info() for
such a mempool. The info is now zeroed first; it was left uninitialized
for drivers without a get_info operation.

The populating thread sets the flag without holding a lock, so set it
with an atomic fetch-or with release ordering, and test it with an
atomic load with acquire ordering. A reader that sees the flag then
also sees the store.

rte_mempool_walk() still passes mempools that are not populated yet to
its callback. Document that the callback must check
rte_mempool_is_created() before querying the mempool driver.

Fixes: 449c49b93a6b ("mempool: support handler operations")
Fixes: ab8a1cc4e3d8 ("mempool: add object usage counts in telemetry")
Cc: stable@dpdk.org

Signed-off-by: Robert Sanford <rsanford@akamai.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 doc/guides/rel_notes/release_26_11.rst |  7 +++++
 lib/mempool/rte_mempool.c              | 37 +++++++++++++++++++++++---
 lib/mempool/rte_mempool.h              | 26 ++++++++++++++++++
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/doc/guides/rel_notes/release_26_11.rst b/doc/guides/rel_notes/release_26_11.rst
index 8ca8379cc6..1d839398b8 100644
--- a/doc/guides/rel_notes/release_26_11.rst
+++ b/doc/guides/rel_notes/release_26_11.rst
@@ -82,6 +82,13 @@ New Features
   but objects within each bulk are not ordered as expected by a stack.
   Furthermore, it is not strictly bounded by its size, but might hold more objects.
 
+* **Added function to check if a mempool is created.**
+
+  Added the ``rte_mempool_is_created()`` function
+  to check whether the store of a mempool has been created.
+  ``rte_mempool_walk()`` callbacks can use it to skip mempools
+  that are not populated yet before querying the mempool driver.
+
 * **Added unplug operation support to DPAA bus.**
 
   Implemented device unplug operation to allow runtime removal of DPAA devices.
diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index d9f339a3c2..6dc0f146a1 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -294,6 +294,22 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
 	}
 }
 
+RTE_EXPORT_SYMBOL(rte_mempool_is_created)
+bool
+rte_mempool_is_created(const struct rte_mempool *mp)
+{
+	/*
+	 * Mempools are on the mempool list before the store is allocated, so
+	 * rte_mempool_dump() and rte_mempool_walk() callbacks can find them
+	 * while another thread populates them.
+	 * Acquire: pairs with the release in mempool_ops_alloc_once().
+	 */
+	return (rte_atomic_load_explicit(
+			(const RTE_ATOMIC(unsigned int) *)&mp->flags,
+			rte_memory_order_acquire) &
+		RTE_MEMPOOL_F_POOL_CREATED) != 0;
+}
+
 static int
 mempool_ops_alloc_once(struct rte_mempool *mp)
 {
@@ -304,7 +320,13 @@ mempool_ops_alloc_once(struct rte_mempool *mp)
 		ret = rte_mempool_ops_alloc(mp);
 		if (ret != 0)
 			return ret;
-		mp->flags |= RTE_MEMPOOL_F_POOL_CREATED;
+		/*
+		 * Release: make the store visible before the flag, see
+		 * rte_mempool_is_created().
+		 */
+		rte_atomic_fetch_or_explicit(
+			(RTE_ATOMIC(unsigned int) *)&mp->flags,
+			RTE_MEMPOOL_F_POOL_CREATED, rte_memory_order_release);
 	}
 	return 0;
 }
@@ -1283,10 +1305,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	unsigned common_count;
 	unsigned cache_count;
 	size_t mem_len = 0;
+	bool created;
 
 	RTE_ASSERT(f != NULL);
 	RTE_ASSERT(mp != NULL);
 
+	/* the mempool driver can only be queried once the store exists */
+	created = rte_mempool_is_created(mp);
+
 	fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
 	fprintf(f, "  flags=%x\n", mp->flags);
 	fprintf(f, "  socket_id=%d\n", mp->socket_id);
@@ -1318,14 +1344,16 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	}
 
 	cache_count = rte_mempool_dump_cache(f, mp);
-	common_count = rte_mempool_ops_get_count(mp);
+	common_count = created ? rte_mempool_ops_get_count(mp) : 0;
 	if ((cache_count + common_count) > mp->size)
 		common_count = mp->size - cache_count;
 	fprintf(f, "  common_pool_count=%u\n", common_count);
 
 	/* sum and dump statistics */
 #ifdef RTE_LIBRTE_MEMPOOL_STATS
-	rte_mempool_ops_get_info(mp, &info);
+	memset(&info, 0, sizeof(info));
+	if (created)
+		rte_mempool_ops_get_info(mp, &info);
 	memset(&sum, 0, sizeof(sum));
 	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE + 1; lcore_id++) {
 		sum.put_bulk += mp->stats[lcore_id].put_bulk;
@@ -1653,7 +1681,8 @@ mempool_info_cb(struct rte_mempool *mp, void *arg)
 			cache_count += cache->len;
 	}
 	rte_tel_data_add_dict_uint(info->d, "total_cache_count", cache_count);
-	common_count = rte_mempool_ops_get_count(mp);
+	/* the mempool driver can only be queried once the store exists */
+	common_count = rte_mempool_is_created(mp) ? rte_mempool_ops_get_count(mp) : 0;
 	if ((cache_count + common_count) > mp->size)
 		common_count = mp->size - cache_count;
 	rte_tel_data_add_dict_uint(info->d, "common_pool_count", common_count);
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index bd58d8a9d5..9bf1220b71 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1325,6 +1325,9 @@ void rte_mempool_stats_reset(struct rte_mempool *mp);
 /**
  * Dump the status of the mempool to a file.
  *
+ * For a mempool that is not populated yet, the mempool driver is not
+ * queried, and the common pool count is reported as 0.
+ *
  * @param f
  *   A pointer to a file for output
  * @param mp
@@ -1791,6 +1794,24 @@ rte_mempool_get_contig_blocks(struct rte_mempool *mp,
 	return ret;
 }
 
+/**
+ * Check if the mempool driver has created the store of a mempool.
+ *
+ * rte_mempool_create_empty() adds a mempool to the mempool list, and
+ * assigns the default mempool driver, before the store exists. The
+ * mempool driver can still be changed with rte_mempool_set_ops_byname()
+ * until the store is created by the first populate call. Functions that
+ * query the mempool driver, such as rte_mempool_avail_count(), must only
+ * be called for a mempool that is created. rte_mempool_walk() callbacks
+ * can see mempools that are not created yet.
+ *
+ * @param mp
+ *   A pointer to the mempool structure.
+ * @return
+ *   true if the store has been created, false otherwise.
+ */
+bool rte_mempool_is_created(const struct rte_mempool *mp);
+
 /**
  * Return the number of entries in the mempool.
  *
@@ -1950,6 +1971,11 @@ uint32_t rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
  * The mempool list is locked while the iterator function runs, so the
  * iterator function must not create or free a mempool.
  *
+ * The iterator function is also called for mempools that are not
+ * populated yet. Such a mempool has no store yet, so the iterator function
+ * must check rte_mempool_is_created() before calling functions that query
+ * the mempool driver, such as rte_mempool_avail_count().
+ *
  * @param func
  *   Iterator function
  * @param arg
-- 
2.34.1

[PATCH v3 3/3] test/mempool: add list walk race test

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

Walk, look up and dump the mempool list on some lcores while other
lcores create and free mempools, in two phases of 500 ms. In the
first phase, mempools are created and freed as fast as possible. In
the second phase, every other mempool is created empty and populated
after a short delay, so that the walkers also find mempools that are
not populated yet. In that phase, the walk callback also dumps each
mempool, and it checks rte_mempool_is_created() before querying the
mempool driver, as documented for rte_mempool_walk().

Without either of the two previous patches, the test crashes. It
needs at least 3 lcores and is skipped otherwise. It adds about one
second to mempool_autotest.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
---
 app/test/test_mempool.c | 144 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 144 insertions(+)

diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c
index 0a4d794346..ac26ef55a6 100644
--- a/app/test/test_mempool.c
+++ b/app/test/test_mempool.c
@@ -839,6 +839,146 @@ test_mempool_flag_non_io_unset_when_populated_with_valid_iova(void)
 
 #pragma pop_macro("RTE_TEST_TRACE_FAILURE")
 
+/*
+ * Walk, look up and dump the mempool list on some lcores while other lcores
+ * create and free mempools. In the first phase the creators free mempools as
+ * fast as possible. In the second phase every other mempool is created empty
+ * and populated after a short delay, so that the walkers also find mempools
+ * that are not populated yet, and dump them.
+ */
+#define LIST_RACE_PHASE_MS	500
+#define LIST_RACE_NB_OBJ	63
+#define LIST_RACE_ELT_SIZE	64
+
+static RTE_ATOMIC(uint32_t) list_race_stop;
+static int list_race_populate_later;
+static FILE *list_race_dump;
+
+static int
+test_mempool_list_race_create(__rte_unused void *arg)
+{
+	char name[RTE_MEMPOOL_NAMESIZE];
+	struct rte_mempool *mp;
+	int populate_later = 0;
+
+	snprintf(name, sizeof(name), "test_list_race_%u", rte_lcore_id());
+	while (rte_atomic_load_explicit(&list_race_stop,
+			rte_memory_order_relaxed) == 0) {
+		if (populate_later) {
+			mp = rte_mempool_create_empty(name, LIST_RACE_NB_OBJ,
+				LIST_RACE_ELT_SIZE, 0, 0, SOCKET_ID_ANY, 0);
+			if (mp == NULL)
+				RET_ERR();
+			rte_delay_us(100);
+			if (rte_mempool_set_ops_byname(mp,
+					rte_mbuf_best_mempool_ops(), NULL) < 0 ||
+					rte_mempool_populate_default(mp) < 0) {
+				rte_mempool_free(mp);
+				RET_ERR();
+			}
+		} else {
+			mp = rte_mempool_create(name, LIST_RACE_NB_OBJ,
+				LIST_RACE_ELT_SIZE, 0, 0, NULL, NULL, NULL, NULL,
+				SOCKET_ID_ANY, 0);
+			if (mp == NULL)
+				RET_ERR();
+		}
+		rte_mempool_free(mp);
+		if (list_race_populate_later)
+			populate_later = !populate_later;
+	}
+	return 0;
+}
+
+static void
+test_mempool_list_race_walk_cb(struct rte_mempool *mp, void *arg)
+{
+	unsigned int *count = arg;
+
+	/* rte_mempool_dump() must handle mempools that are not populated yet */
+	if (list_race_dump != NULL)
+		rte_mempool_dump(list_race_dump, mp);
+
+	/* as documented for rte_mempool_walk(), check this first */
+	if (!rte_mempool_is_created(mp))
+		return;
+	/* access the store */
+	*count += mp->size + rte_mempool_avail_count(mp);
+}
+
+static int
+test_mempool_list_race_walk(__rte_unused void *arg)
+{
+	unsigned int count = 0;
+
+	while (rte_atomic_load_explicit(&list_race_stop,
+			rte_memory_order_relaxed) == 0) {
+		rte_mempool_walk(test_mempool_list_race_walk_cb, &count);
+		if (rte_mempool_lookup("test_list_race_none") != NULL)
+			RET_ERR();
+		if (list_race_dump != NULL)
+			rte_mempool_list_dump(list_race_dump);
+	}
+	return 0;
+}
+
+static int
+test_mempool_list_race_phase(int populate_later, FILE *dump)
+{
+	unsigned int nb_creators = (rte_lcore_count() - 1) / 2;
+	unsigned int lcore_id, i = 0;
+	int ret = 0;
+
+	list_race_populate_later = populate_later;
+	list_race_dump = dump;
+	rte_atomic_store_explicit(&list_race_stop, 0, rte_memory_order_relaxed);
+	RTE_LCORE_FOREACH_WORKER(lcore_id) {
+		lcore_function_t *func = i++ < nb_creators ?
+			test_mempool_list_race_create :
+			test_mempool_list_race_walk;
+
+		if (rte_eal_remote_launch(func, NULL, lcore_id) != 0)
+			ret = -1;
+	}
+
+	rte_delay_ms(LIST_RACE_PHASE_MS);
+	rte_atomic_store_explicit(&list_race_stop, 1, rte_memory_order_relaxed);
+
+	RTE_LCORE_FOREACH_WORKER(lcore_id) {
+		if (rte_eal_wait_lcore(lcore_id) < 0)
+			ret = -1;
+	}
+	return ret;
+}
+
+static int
+test_mempool_list_race(void)
+{
+	FILE *f;
+	int ret;
+
+	if (rte_lcore_count() < 3) {
+		printf("%s skipped: needs at least 3 lcores\n", __func__);
+		return 0;
+	}
+
+	/* mempools freed while the list is walked */
+	ret = test_mempool_list_race_phase(0, NULL);
+	if (ret != 0) {
+		LOG_ERR();
+		return ret;
+	}
+
+	/* mempools not populated yet while the list is walked and dumped */
+	f = fopen("/dev/null", "w");
+	ret = test_mempool_list_race_phase(1, f);
+	if (f != NULL)
+		fclose(f);
+	if (ret != 0)
+		LOG_ERR();
+	return ret;
+}
+
 static int
 test_mempool(void)
 {
@@ -1105,6 +1245,10 @@ test_mempool(void)
 	if (test_mempool_flag_non_io_unset_when_populated_with_valid_iova() < 0)
 		GOTO_ERR(ret, err);
 
+	/* test walking the mempool list while mempools are created and freed */
+	if (test_mempool_list_race() < 0)
+		GOTO_ERR(ret, err);
+
 	rte_mempool_list_dump(stdout);
 
 	ret = 0;
-- 
2.34.1

RE: [PATCH v3 3/3] test/mempool: add list walk race test

[Morten Brørup]

> +	f = fopen("/dev/null", "w");

"/dev/null" is "NUL" in Windows. Look here for inspiration:
https://github.com/DPDK/dpdk/blob/v26.11-rc2/app/test/test_cmdline_lib.c#L24-L28

With that fixed,
Acked-by: Morten Brørup <mb@smartsharesystems.com>

[PATCH v4 0/3] mempool: fix races between list walk and create/free

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

A thread walking the mempool list with rte_mempool_walk(),
rte_mempool_lookup() or rte_mempool_list_dump() can crash while other
threads create and free mempools. We hit this in production, in a
process that walks the mempools while other threads create and free
them.

There are two races:

- rte_mempool_free() unlinks and frees the list entry without the
  mempool list lock that the walkers hold, so a walker can reach a
  freed entry or mempool. Patch 1 takes the lock in rte_mempool_free(),
  as rte_mempool_create_empty() already does. A rte_mempool_walk()
  callback can therefore no longer free a mempool; this is documented.

- A mempool is on the list before its store exists, but
  rte_mempool_dump() queries the mempool driver, which reads the store.
  So rte_mempool_list_dump() and the telemetry /mempool/info handler
  crash on such a mempool. Patch 2 adds rte_mempool_is_created(),
  makes them report the mempool without querying the driver until it
  returns true, and documents that rte_mempool_walk() callbacks must
  check it before querying the driver.

Patch 3 adds a test to mempool_autotest. On x86 with 4 or 8 lcores,
it crashed in all 20 runs without patch 1, without patch 2, or
without both, and passed in all 20 runs with both patches.

v4:
- Patch 3, as suggested by Morten Brørup: the dump goes to "NUL" instead
  of "/dev/null" on Windows. Acked-by added.
- Rebased on main.
- Patches 1 and 2: unchanged.

v3:
- Patch 2, as suggested by Morten Brørup: the helper that tests
  RTE_MEMPOOL_F_POOL_CREATED is now the public function
  rte_mempool_is_created(), and the rte_mempool_walk() documentation
  refers to it instead of the flag. Release note added. Acked-by added.
- Patch 3: the walk callback uses rte_mempool_is_created().
- Patch 1: unchanged.

v2:
- Patch 2 reworked as suggested by Morten Brørup: rte_mempool_walk()
  no longer skips mempools that are not populated yet. Instead,
  rte_mempool_dump() and the telemetry callback report them without
  querying the mempool driver, and the rte_mempool_walk() documentation
  asks callbacks to check RTE_MEMPOOL_F_POOL_CREATED. The flag is now
  set and tested with release/acquire atomics instead of plain accesses
  with fences. New title.
- Patch 3: the walk callback dumps every mempool, and checks the flag
  before querying the driver. The test waits 100 us instead of 10 us
  before populating, so that it still finds unpopulated mempools
  reliably.
- Patch 1: unchanged, Reviewed-by added.

Robert Sanford (3):
  mempool: fix race between list walk and free
  mempool: fix dump of unpopulated pool
  test/mempool: add list walk race test

 app/test/test_mempool.c                | 150 +++++++++++++++++++++++++
 doc/guides/rel_notes/release_26_11.rst |   7 ++
 lib/mempool/rte_mempool.c              |  45 +++++++-
 lib/mempool/rte_mempool.h              |  29 +++++
 4 files changed, 227 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] mempool: fix race between list walk and free

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

rte_mempool_walk(), rte_mempool_lookup() and rte_mempool_list_dump()
traverse the mempool list holding only the mempool list lock.
rte_mempool_create_empty() inserts into the list holding both the
mempool list lock and the tailq lock, but rte_mempool_free() unlinks
and frees the list entry holding only the tailq lock. So a mempool can
be unlinked and freed, together with its list entry, while another
thread is walking the list. The walker then reads freed memory. Since
freed heap memory is zeroed, the walk callback is passed a NULL
mempool, or rte_mempool_lookup() compares the name of one, and the
process crashes.

Take the mempool list lock in rte_mempool_free() as well, in the same
order as rte_mempool_create_empty(). As a result, a rte_mempool_walk()
callback must not free a mempool, as it already could not create one.
Document this.

Fixes: a3791ff0a3f8 ("mempool: introduce a function to free a pool")
Cc: stable@dpdk.org

Signed-off-by: Robert Sanford <rsanford@akamai.com>
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/mempool/rte_mempool.c | 8 ++++++++
 lib/mempool/rte_mempool.h | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index 069aabaa70..d9f339a3c2 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -729,6 +729,13 @@ rte_mempool_free(struct rte_mempool *mp)
 		return;
 
 	mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempool_list);
+	/*
+	 * Take the mempool list lock as well, like rte_mempool_create_empty(),
+	 * so that the entry is not unlinked and freed while
+	 * rte_mempool_walk(), rte_mempool_lookup() or rte_mempool_list_dump()
+	 * is traversing the list.
+	 */
+	rte_mcfg_mempool_write_lock();
 	rte_mcfg_tailq_write_lock();
 	/* find out tailq entry */
 	TAILQ_FOREACH(te, mempool_list, next) {
@@ -741,6 +748,7 @@ rte_mempool_free(struct rte_mempool *mp)
 		rte_free(te);
 	}
 	rte_mcfg_tailq_write_unlock();
+	rte_mcfg_mempool_write_unlock();
 
 	mempool_event_callback_invoke(RTE_MEMPOOL_EVENT_DESTROY, mp);
 	rte_mempool_trace_free(mp);
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index b34cd893b1..bd58d8a9d5 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1947,6 +1947,9 @@ uint32_t rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
 /**
  * Walk list of all memory pools
  *
+ * The mempool list is locked while the iterator function runs, so the
+ * iterator function must not create or free a mempool.
+ *
  * @param func
  *   Iterator function
  * @param arg
-- 
2.34.1

[PATCH v4 2/3] mempool: fix dump of unpopulated pool

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

rte_mempool_create_empty() adds a mempool to the mempool list before
the mempool has a store. The store is allocated, and
RTE_MEMPOOL_F_POOL_CREATED is set, by the first populate call. In
between, rte_mempool_dump() queries the mempool driver for the number
of objects in the store. With the ring driver this dereferences a NULL
ring, and the process crashes. rte_mempool_list_dump() calls
rte_mempool_dump() for every mempool on the list, so it crashes when
another thread has created a mempool but not populated it yet. The
telemetry /mempool/info handler has the same problem: its
rte_mempool_walk() callback queries the driver for the same count.

Add rte_mempool_is_created(), which tests RTE_MEMPOOL_F_POOL_CREATED.
In rte_mempool_dump() and in the telemetry callback, query the mempool
driver only when it returns true, and report a common
pool count of 0 otherwise, since the mempool has no objects yet. The
rest of the mempool information is still reported. In statistics
builds, rte_mempool_dump() also skips rte_mempool_ops_get_info() for
such a mempool. The info is now zeroed first; it was left uninitialized
for drivers without a get_info operation.

The populating thread sets the flag without holding a lock, so set it
with an atomic fetch-or with release ordering, and test it with an
atomic load with acquire ordering. A reader that sees the flag then
also sees the store.

rte_mempool_walk() still passes mempools that are not populated yet to
its callback. Document that the callback must check
rte_mempool_is_created() before querying the mempool driver.

Fixes: 449c49b93a6b ("mempool: support handler operations")
Fixes: ab8a1cc4e3d8 ("mempool: add object usage counts in telemetry")
Cc: stable@dpdk.org

Signed-off-by: Robert Sanford <rsanford@akamai.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 doc/guides/rel_notes/release_26_11.rst |  7 +++++
 lib/mempool/rte_mempool.c              | 37 +++++++++++++++++++++++---
 lib/mempool/rte_mempool.h              | 26 ++++++++++++++++++
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/doc/guides/rel_notes/release_26_11.rst b/doc/guides/rel_notes/release_26_11.rst
index e62f273aad..2f50a602e8 100644
--- a/doc/guides/rel_notes/release_26_11.rst
+++ b/doc/guides/rel_notes/release_26_11.rst
@@ -82,6 +82,13 @@ New Features
   but objects within each bulk are not ordered as expected by a stack.
   Furthermore, it is not strictly bounded by its size, but might hold more objects.
 
+* **Added function to check if a mempool is created.**
+
+  Added the ``rte_mempool_is_created()`` function
+  to check whether the store of a mempool has been created.
+  ``rte_mempool_walk()`` callbacks can use it to skip mempools
+  that are not populated yet before querying the mempool driver.
+
 * **Added unplug operation support to DPAA bus.**
 
   Implemented device unplug operation to allow runtime removal of DPAA devices.
diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index d9f339a3c2..6dc0f146a1 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -294,6 +294,22 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
 	}
 }
 
+RTE_EXPORT_SYMBOL(rte_mempool_is_created)
+bool
+rte_mempool_is_created(const struct rte_mempool *mp)
+{
+	/*
+	 * Mempools are on the mempool list before the store is allocated, so
+	 * rte_mempool_dump() and rte_mempool_walk() callbacks can find them
+	 * while another thread populates them.
+	 * Acquire: pairs with the release in mempool_ops_alloc_once().
+	 */
+	return (rte_atomic_load_explicit(
+			(const RTE_ATOMIC(unsigned int) *)&mp->flags,
+			rte_memory_order_acquire) &
+		RTE_MEMPOOL_F_POOL_CREATED) != 0;
+}
+
 static int
 mempool_ops_alloc_once(struct rte_mempool *mp)
 {
@@ -304,7 +320,13 @@ mempool_ops_alloc_once(struct rte_mempool *mp)
 		ret = rte_mempool_ops_alloc(mp);
 		if (ret != 0)
 			return ret;
-		mp->flags |= RTE_MEMPOOL_F_POOL_CREATED;
+		/*
+		 * Release: make the store visible before the flag, see
+		 * rte_mempool_is_created().
+		 */
+		rte_atomic_fetch_or_explicit(
+			(RTE_ATOMIC(unsigned int) *)&mp->flags,
+			RTE_MEMPOOL_F_POOL_CREATED, rte_memory_order_release);
 	}
 	return 0;
 }
@@ -1283,10 +1305,14 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	unsigned common_count;
 	unsigned cache_count;
 	size_t mem_len = 0;
+	bool created;
 
 	RTE_ASSERT(f != NULL);
 	RTE_ASSERT(mp != NULL);
 
+	/* the mempool driver can only be queried once the store exists */
+	created = rte_mempool_is_created(mp);
+
 	fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
 	fprintf(f, "  flags=%x\n", mp->flags);
 	fprintf(f, "  socket_id=%d\n", mp->socket_id);
@@ -1318,14 +1344,16 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
 	}
 
 	cache_count = rte_mempool_dump_cache(f, mp);
-	common_count = rte_mempool_ops_get_count(mp);
+	common_count = created ? rte_mempool_ops_get_count(mp) : 0;
 	if ((cache_count + common_count) > mp->size)
 		common_count = mp->size - cache_count;
 	fprintf(f, "  common_pool_count=%u\n", common_count);
 
 	/* sum and dump statistics */
 #ifdef RTE_LIBRTE_MEMPOOL_STATS
-	rte_mempool_ops_get_info(mp, &info);
+	memset(&info, 0, sizeof(info));
+	if (created)
+		rte_mempool_ops_get_info(mp, &info);
 	memset(&sum, 0, sizeof(sum));
 	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE + 1; lcore_id++) {
 		sum.put_bulk += mp->stats[lcore_id].put_bulk;
@@ -1653,7 +1681,8 @@ mempool_info_cb(struct rte_mempool *mp, void *arg)
 			cache_count += cache->len;
 	}
 	rte_tel_data_add_dict_uint(info->d, "total_cache_count", cache_count);
-	common_count = rte_mempool_ops_get_count(mp);
+	/* the mempool driver can only be queried once the store exists */
+	common_count = rte_mempool_is_created(mp) ? rte_mempool_ops_get_count(mp) : 0;
 	if ((cache_count + common_count) > mp->size)
 		common_count = mp->size - cache_count;
 	rte_tel_data_add_dict_uint(info->d, "common_pool_count", common_count);
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index bd58d8a9d5..9bf1220b71 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1325,6 +1325,9 @@ void rte_mempool_stats_reset(struct rte_mempool *mp);
 /**
  * Dump the status of the mempool to a file.
  *
+ * For a mempool that is not populated yet, the mempool driver is not
+ * queried, and the common pool count is reported as 0.
+ *
  * @param f
  *   A pointer to a file for output
  * @param mp
@@ -1791,6 +1794,24 @@ rte_mempool_get_contig_blocks(struct rte_mempool *mp,
 	return ret;
 }
 
+/**
+ * Check if the mempool driver has created the store of a mempool.
+ *
+ * rte_mempool_create_empty() adds a mempool to the mempool list, and
+ * assigns the default mempool driver, before the store exists. The
+ * mempool driver can still be changed with rte_mempool_set_ops_byname()
+ * until the store is created by the first populate call. Functions that
+ * query the mempool driver, such as rte_mempool_avail_count(), must only
+ * be called for a mempool that is created. rte_mempool_walk() callbacks
+ * can see mempools that are not created yet.
+ *
+ * @param mp
+ *   A pointer to the mempool structure.
+ * @return
+ *   true if the store has been created, false otherwise.
+ */
+bool rte_mempool_is_created(const struct rte_mempool *mp);
+
 /**
  * Return the number of entries in the mempool.
  *
@@ -1950,6 +1971,11 @@ uint32_t rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
  * The mempool list is locked while the iterator function runs, so the
  * iterator function must not create or free a mempool.
  *
+ * The iterator function is also called for mempools that are not
+ * populated yet. Such a mempool has no store yet, so the iterator function
+ * must check rte_mempool_is_created() before calling functions that query
+ * the mempool driver, such as rte_mempool_avail_count().
+ *
  * @param func
  *   Iterator function
  * @param arg
-- 
2.34.1

[PATCH v4 3/3] test/mempool: add list walk race test

[Robert Sanford]

From: Robert Sanford <rsanford@akamai.com>

Walk, look up and dump the mempool list on some lcores while other
lcores create and free mempools, in two phases of 500 ms. In the
first phase, mempools are created and freed as fast as possible. In
the second phase, every other mempool is created empty and populated
after a short delay, so that the walkers also find mempools that are
not populated yet. In that phase, the walk callback also dumps each
mempool, and it checks rte_mempool_is_created() before querying the
mempool driver, as documented for rte_mempool_walk().

Without either of the two previous patches, the test crashes. It
needs at least 3 lcores and is skipped otherwise. It adds about one
second to mempool_autotest.

Signed-off-by: Robert Sanford <rsanford@akamai.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 app/test/test_mempool.c | 150 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 150 insertions(+)

diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c
index 0a4d794346..0edb691b56 100644
--- a/app/test/test_mempool.c
+++ b/app/test/test_mempool.c
@@ -839,6 +839,152 @@ test_mempool_flag_non_io_unset_when_populated_with_valid_iova(void)
 
 #pragma pop_macro("RTE_TEST_TRACE_FAILURE")
 
+/*
+ * Walk, look up and dump the mempool list on some lcores while other lcores
+ * create and free mempools. In the first phase the creators free mempools as
+ * fast as possible. In the second phase every other mempool is created empty
+ * and populated after a short delay, so that the walkers also find mempools
+ * that are not populated yet, and dump them.
+ */
+#define LIST_RACE_PHASE_MS	500
+#define LIST_RACE_NB_OBJ	63
+#define LIST_RACE_ELT_SIZE	64
+
+#ifndef RTE_EXEC_ENV_WINDOWS
+#define LIST_RACE_DUMP_FILE	"/dev/null"
+#else
+#define LIST_RACE_DUMP_FILE	"NUL"
+#endif
+
+static RTE_ATOMIC(uint32_t) list_race_stop;
+static int list_race_populate_later;
+static FILE *list_race_dump;
+
+static int
+test_mempool_list_race_create(__rte_unused void *arg)
+{
+	char name[RTE_MEMPOOL_NAMESIZE];
+	struct rte_mempool *mp;
+	int populate_later = 0;
+
+	snprintf(name, sizeof(name), "test_list_race_%u", rte_lcore_id());
+	while (rte_atomic_load_explicit(&list_race_stop,
+			rte_memory_order_relaxed) == 0) {
+		if (populate_later) {
+			mp = rte_mempool_create_empty(name, LIST_RACE_NB_OBJ,
+				LIST_RACE_ELT_SIZE, 0, 0, SOCKET_ID_ANY, 0);
+			if (mp == NULL)
+				RET_ERR();
+			rte_delay_us(100);
+			if (rte_mempool_set_ops_byname(mp,
+					rte_mbuf_best_mempool_ops(), NULL) < 0 ||
+					rte_mempool_populate_default(mp) < 0) {
+				rte_mempool_free(mp);
+				RET_ERR();
+			}
+		} else {
+			mp = rte_mempool_create(name, LIST_RACE_NB_OBJ,
+				LIST_RACE_ELT_SIZE, 0, 0, NULL, NULL, NULL, NULL,
+				SOCKET_ID_ANY, 0);
+			if (mp == NULL)
+				RET_ERR();
+		}
+		rte_mempool_free(mp);
+		if (list_race_populate_later)
+			populate_later = !populate_later;
+	}
+	return 0;
+}
+
+static void
+test_mempool_list_race_walk_cb(struct rte_mempool *mp, void *arg)
+{
+	unsigned int *count = arg;
+
+	/* rte_mempool_dump() must handle mempools that are not populated yet */
+	if (list_race_dump != NULL)
+		rte_mempool_dump(list_race_dump, mp);
+
+	/* as documented for rte_mempool_walk(), check this first */
+	if (!rte_mempool_is_created(mp))
+		return;
+	/* access the store */
+	*count += mp->size + rte_mempool_avail_count(mp);
+}
+
+static int
+test_mempool_list_race_walk(__rte_unused void *arg)
+{
+	unsigned int count = 0;
+
+	while (rte_atomic_load_explicit(&list_race_stop,
+			rte_memory_order_relaxed) == 0) {
+		rte_mempool_walk(test_mempool_list_race_walk_cb, &count);
+		if (rte_mempool_lookup("test_list_race_none") != NULL)
+			RET_ERR();
+		if (list_race_dump != NULL)
+			rte_mempool_list_dump(list_race_dump);
+	}
+	return 0;
+}
+
+static int
+test_mempool_list_race_phase(int populate_later, FILE *dump)
+{
+	unsigned int nb_creators = (rte_lcore_count() - 1) / 2;
+	unsigned int lcore_id, i = 0;
+	int ret = 0;
+
+	list_race_populate_later = populate_later;
+	list_race_dump = dump;
+	rte_atomic_store_explicit(&list_race_stop, 0, rte_memory_order_relaxed);
+	RTE_LCORE_FOREACH_WORKER(lcore_id) {
+		lcore_function_t *func = i++ < nb_creators ?
+			test_mempool_list_race_create :
+			test_mempool_list_race_walk;
+
+		if (rte_eal_remote_launch(func, NULL, lcore_id) != 0)
+			ret = -1;
+	}
+
+	rte_delay_ms(LIST_RACE_PHASE_MS);
+	rte_atomic_store_explicit(&list_race_stop, 1, rte_memory_order_relaxed);
+
+	RTE_LCORE_FOREACH_WORKER(lcore_id) {
+		if (rte_eal_wait_lcore(lcore_id) < 0)
+			ret = -1;
+	}
+	return ret;
+}
+
+static int
+test_mempool_list_race(void)
+{
+	FILE *f;
+	int ret;
+
+	if (rte_lcore_count() < 3) {
+		printf("%s skipped: needs at least 3 lcores\n", __func__);
+		return 0;
+	}
+
+	/* mempools freed while the list is walked */
+	ret = test_mempool_list_race_phase(0, NULL);
+	if (ret != 0) {
+		LOG_ERR();
+		return ret;
+	}
+
+	/* mempools not populated yet while the list is walked and dumped */
+	f = fopen(LIST_RACE_DUMP_FILE, "w");
+	ret = test_mempool_list_race_phase(1, f);
+	if (f != NULL)
+		fclose(f);
+	if (ret != 0)
+		LOG_ERR();
+	return ret;
+}
+
 static int
 test_mempool(void)
 {
@@ -1105,6 +1251,10 @@ test_mempool(void)
 	if (test_mempool_flag_non_io_unset_when_populated_with_valid_iova() < 0)
 		GOTO_ERR(ret, err);
 
+	/* test walking the mempool list while mempools are created and freed */
+	if (test_mempool_list_race() < 0)
+		GOTO_ERR(ret, err);
+
 	rte_mempool_list_dump(stdout);
 
 	ret = 0;
-- 
2.34.1