From b173e4353fe55c42ee7f77277e13106021ba5678 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Mon, 4 Jun 2018 09:25:14 -0700 Subject: [PATCH] kernel/queue: Fix spurious NULL exit condition when using timeouts The queue loop when CONFIG_POLL is in used has an inherent race between the return of k_poll() and the inspection of the list where no lock can be held. Other contending readers of the same queue can sneak in and steal the item out of the list before the current thread gets to the sys_sflist_get() call, and the current loop will (if it has a timeout) spuriously return NULL before the timeout expires. It's not even a hard race to exercise. Consider three threads at different priorities: High (which can be an ISR too), Mid, and Low: 1. Mid and Low both enter k_queue_get() and sleep inside k_poll() on an empty queue. 2. High comes along and calls k_queue_insert(). The queue code then wakes up Mid, and reschedules, but because High is still running Mid doesn't get to run yet. 3. High inserts a SECOND item. The queue then unpends the next thread in the list (Low), and readies it to run. But as before, it won't be scheduled yet. 4. Now High sleeps (or if it's an interrupt, exits), and Mid gets to run. It dequeues and returns the item it was delivered normally. 5. But Mid is still running! So it re-enters the loop it's sitting in and calls k_queue_get() again, which sees and returns the second item in the queue synchronously. Then it calls it a third time and goes to sleep because the queue is empty. 6. Finally, Low wakes up to find an empty queue, and returns NULL despite the fact that the timeout hadn't expired. The fix is simple enough: check the timeout expiration inside the loop so we don't return early. Signed-off-by: Andy Ross --- kernel/queue.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/kernel/queue.c b/kernel/queue.c index 78cb1a02b01..dafb50675a0 100644 --- a/kernel/queue.c +++ b/kernel/queue.c @@ -274,31 +274,39 @@ void k_queue_merge_slist(struct k_queue *queue, sys_slist_t *list) static void *k_queue_poll(struct k_queue *queue, s32_t timeout) { struct k_poll_event event; - int err; + int err, elapsed = 0, done = 0; unsigned int key; void *val; + u32_t start; k_poll_event_init(&event, K_POLL_TYPE_FIFO_DATA_AVAILABLE, K_POLL_MODE_NOTIFY_ONLY, queue); + if (timeout != K_FOREVER) { + start = k_uptime_get_32(); + } + do { event.state = K_POLL_STATE_NOT_READY; - err = k_poll(&event, 1, timeout); - if (err) { + err = k_poll(&event, 1, timeout - elapsed); + + if (err && err != -EAGAIN) { return NULL; } - __ASSERT_NO_MSG(event.state == - K_POLL_STATE_FIFO_DATA_AVAILABLE); - /* sys_sflist_* aren't threadsafe, so must be always protected * by irq_lock. */ key = irq_lock(); val = z_queue_node_peek(sys_sflist_get(&queue->data_q), true); irq_unlock(key); - } while (!val && timeout == K_FOREVER); + + if (!val && timeout != K_FOREVER) { + elapsed = k_uptime_get_32() - start; + done = elapsed > timeout; + } + } while (!val && !done); return val; }