From 75398d2c38d7edab6695d7c5746e1caa8dbd4545 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Thu, 24 May 2018 13:27:35 -0700 Subject: [PATCH] kernel/mempool: Handle transient failure condition The sys_mem_pool implementation has a subtle error case where it detected a simultaneous allocation after having released the lock, in which case exactly one of the racing allocators will return with -EAGAIN (the other one suceeds of course). I documented this condition at the lower level, but forgot to actually handle it at the k_mem_pool level where we want to retry once before going to sleep, as it doesn't generally represent an empty heap. It got caught by code auditing in: https://github.com/zephyrproject-rtos/zephyr/issues/6757 (Full disclosure: I tested this by whiteboxing the first failure. I wasn't able to put together a rig to reliably exercise the actual race.) This patch also fixes a noop thinko in the return logic in the same function, which contained: (ret == -EAGAIN) || (ret && ret != -ENOMEM) The first term is needless and implied by the second. Signed-off-by: Andy Ross --- kernel/mempool.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/kernel/mempool.c b/kernel/mempool.c index 3a3579934ad..7ee05acbbdf 100644 --- a/kernel/mempool.c +++ b/kernel/mempool.c @@ -62,14 +62,32 @@ int k_mem_pool_alloc(struct k_mem_pool *p, struct k_mem_block *block, while (1) { u32_t level_num, block_num; - ret = _sys_mem_pool_block_alloc(&p->base, size, &level_num, - &block_num, &block->data); + /* There is a "managed race" in alloc that can fail + * (albeit in a well-defined way, see comments there) + * with -EAGAIN when simultaneous allocations happen. + * Retry exactly once before sleeping to resolve it. + * If we're so contended that it fails twice, then we + * clearly want to block. + */ + for (int i = 0; i < 2; i++) { + ret = _sys_mem_pool_block_alloc(&p->base, size, + &level_num, &block_num, + &block->data); + if (ret != -EAGAIN) { + break; + } + } + + if (ret == -EAGAIN) { + ret = -ENOMEM; + } + block->id.pool = pool_id(p); block->id.level = level_num; block->id.block = block_num; if (ret == 0 || timeout == K_NO_WAIT || - ret == -EAGAIN || (ret && ret != -ENOMEM)) { + (ret && ret != -ENOMEM)) { return ret; }