Skip to content

extend dlist to detect unlinked nodes and avoid double-remove bugs #12248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 23, 2019
Merged

extend dlist to detect unlinked nodes and avoid double-remove bugs #12248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ee84022
kernel: thread: correct type of dlist node
pabigot Jan 4, 2019
ecea4d9
kernel: sched: fix empty list detection
pabigot Jan 4, 2019
123b697
sys: dlist: Add sys_dnode_is_linked
pabigot Dec 30, 2018
ba1f4a2
kernel: timeout: remove local fix for double-remove
pabigot Dec 30, 2018
677f9da
kernel: poll: fix double-remove of node
pabigot Dec 30, 2018
f364e2a
kernel: timeout: detect inactive timeouts using dnode linked state
pabigot Jan 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions include/kernel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ struct _thread_base {

/* this thread's entry in a ready/wait queue */
union {
sys_dlist_t qnode_dlist;
sys_dnode_t qnode_dlist;
struct rbnode qnode_rb;
};

Expand Down Expand Up @@ -874,9 +874,6 @@ __syscall void k_thread_start(k_tid_t thread);
/* timeout has timed out and is not on _timeout_q anymore */
#define _EXPIRED (-2)

/* timeout is not in use */
#define _INACTIVE (-1)

struct _static_thread_data {
struct k_thread *init_thread;
k_thread_stack_t *init_stack;
Expand Down Expand Up @@ -1335,7 +1332,7 @@ struct k_timer {

#define _K_TIMER_INITIALIZER(obj, expiry, stop) \
{ \
.timeout.dticks = _INACTIVE, \
.timeout.dticks = 0, \
.timeout.fn = _timer_expiration_handler, \
.wait_q = _WAIT_Q_INIT(&obj.wait_q), \
.expiry_fn = expiry, \
Expand Down
30 changes: 29 additions & 1 deletion include/misc/dlist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ typedef struct _dnode sys_dnode_t;
__cns = SYS_DLIST_PEEK_NEXT_CONTAINER(__dl, __cn, __n))

/**
* @brief initialize list
* @brief initialize list to its empty state
*
* @param list the doubly-linked list
*
Expand All @@ -196,6 +196,33 @@ static inline void sys_dlist_init(sys_dlist_t *list)

#define SYS_DLIST_STATIC_INIT(ptr_to_list) { {(ptr_to_list)}, {(ptr_to_list)} }

/**
* @brief initialize node to its state when not in a list
*
* @param node the node
*
* @return N/A
*/

static inline void sys_dnode_init(sys_dnode_t *node)
{
node->next = NULL;
node->prev = NULL;
}

/**
* @brief check if a node is a member of any list
*
* @param node the node
*
* @return true if node is linked into a list, false if it is not
*/

static inline bool sys_dnode_is_linked(const sys_dnode_t *node)
{
return node->next != NULL;
}

/**
* @brief check if a node is the list's head
*
Expand Down Expand Up @@ -500,6 +527,7 @@ static inline void sys_dlist_remove(sys_dnode_t *node)
{
node->prev->next = node->next;
node->next->prev = node->prev;
sys_dnode_init(node);
}

/**
Expand Down
7 changes: 2 additions & 5 deletions kernel/include/ksched.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#define ZEPHYR_KERNEL_INCLUDE_KSCHED_H_

#include <kernel_structs.h>
#include <timeout_q.h>
#include <tracing.h>
#include <stdbool.h>

Expand Down Expand Up @@ -83,11 +84,7 @@ static inline int _is_thread_prevented_from_running(struct k_thread *thread)

static inline bool _is_thread_timeout_active(struct k_thread *thread)
{
#ifdef CONFIG_SYS_CLOCK_EXISTS
return thread->base.timeout.dticks != _INACTIVE;
#else
return false;
#endif
return !_is_inactive_timeout(&thread->base.timeout);
}

static inline bool _is_thread_ready(struct k_thread *thread)
Expand Down
8 changes: 7 additions & 1 deletion kernel/include/timeout_q.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,18 @@ extern "C" {

static inline void _init_timeout(struct _timeout *t, _timeout_func_t fn)
{
t->dticks = _INACTIVE;
sys_dnode_init(&t->node);
}

void _add_timeout(struct _timeout *to, _timeout_func_t fn, s32_t ticks);

int _abort_timeout(struct _timeout *to);

static inline bool _is_inactive_timeout(struct _timeout *t)
Copy link
Contributor

@andyross andyross Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not particularly relevant, but this was always a bad API (a true return indicates a false state) that never got fixed up during all the refactoring. I wouldn't cry to see this patch change it to z_timeout_active() or whatever.

Copy link
Contributor Author

@pabigot pabigot Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was partly because of the legacy of _EXPIRED: timeouts could be in three states, not two. This function tests that the state is exactly what _INACTIVE indicated.

A true result indicates that the predicate identified by the function name is true. Why is that a bad API? We use sys_dlist_is_empty(), not sys_dlist_is_nonempty().

The whole timeout API uses prefix underscore. While I'd rather that wasn't the case (violates C reserved identifiers for the non-static functions) I wouldn't want to change just one function, nor change all of them, in this PR.

Copy link
Contributor

@andyross andyross Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A true result indicates that the predicate identified by the function name is true

I don't think that not incorrect, exactly, but it's not innapropriate to argue against the ellision of unommitted negatives either.

No seriously, double negatives in API choices are bad, especially in predicates. The base adjective for that state is "active". True should mean "active", false should mean "inactive".

{
return !sys_dnode_is_linked(&t->node);
}

static inline void _init_thread_timeout(struct _thread_base *thread_base)
{
_init_timeout(&thread_base->timeout, NULL);
Expand Down Expand Up @@ -58,6 +63,7 @@ s32_t z_timeout_remaining(struct _timeout *timeout);
#define _init_thread_timeout(t) do {} while (0)
#define _add_thread_timeout(th, to) do {} while (0 && (void *)to && (void *)th)
#define _abort_thread_timeout(t) (0)
#define _is_inactive_timeout(t) 0
#define _get_next_timeout_expiry() (K_FOREVER)
#define z_set_timeout_expiry(t, i) do {} while (0)

Expand Down
11 changes: 8 additions & 3 deletions kernel/poll.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,20 +133,22 @@ static inline int register_event(struct k_poll_event *event,
/* must be called with interrupts locked */
static inline void clear_event_registration(struct k_poll_event *event)
{
bool remove = false;

event->poller = NULL;

switch (event->type) {
case K_POLL_TYPE_SEM_AVAILABLE:
__ASSERT(event->sem != NULL, "invalid semaphore\n");
sys_dlist_remove(&event->_node);
remove = true;
break;
case K_POLL_TYPE_DATA_AVAILABLE:
__ASSERT(event->queue != NULL, "invalid queue\n");
sys_dlist_remove(&event->_node);
remove = true;
break;
case K_POLL_TYPE_SIGNAL:
__ASSERT(event->signal != NULL, "invalid poll signal\n");
sys_dlist_remove(&event->_node);
remove = true;
break;
case K_POLL_TYPE_IGNORE:
/* nothing to do */
Expand All @@ -155,6 +157,9 @@ static inline void clear_event_registration(struct k_poll_event *event)
__ASSERT(false, "invalid event type\n");
break;
}
if (remove && sys_dnode_is_linked(&event->_node)) {
sys_dlist_remove(&event->_node);
}
Copy link
Contributor

@andyross andyross Dec 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good catch, and it's surprising that it didn't cause failures already (I guess the existing tests would all exercise the double remove only in circumstances where the list hadn't otherwise changed and the action would be idempotent). I wonder idly if it's not a better idea to put that test into sys_dlist_remove() as long as we're eating the extra cycles...

Copy link
Contributor Author

@pabigot pabigot Dec 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sys_dlist_remove already documents that the node must be part of the list, and in most situations that's true (and known at the call-site). So I don't think there's reason to add a check in that function.

}

/* must be called with interrupts locked */
Expand Down
20 changes: 16 additions & 4 deletions kernel/sched.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,9 +591,13 @@ void _priq_dumb_remove(sys_dlist_t *pq, struct k_thread *thread)

struct k_thread *_priq_dumb_best(sys_dlist_t *pq)
{
struct k_thread *t = NULL;
Copy link
Contributor

@andyross andyross Jan 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct, but can you clarity the explanation? It was a bug, call it a bug, e.g:

kernel/sched: Fix empty list detection

CONTAINER_OF() on a NULL pointer returns some offset around NULL and
not another NULL pointer.  We have to check for that ourselves.

This only worked because the dnode happened to be at the start of the struct.

sys_dnode_t *n = sys_dlist_peek_head(pq);

return CONTAINER_OF(n, struct k_thread, base.qnode_dlist);
if (n != NULL) {
t = CONTAINER_OF(n, struct k_thread, base.qnode_dlist);
}
return t;
}

bool _priq_rb_lessthan(struct rbnode *a, struct rbnode *b)
Expand Down Expand Up @@ -648,9 +652,13 @@ void _priq_rb_remove(struct _priq_rb *pq, struct k_thread *thread)

struct k_thread *_priq_rb_best(struct _priq_rb *pq)
{
struct k_thread *t = NULL;
struct rbnode *n = rb_get_min(&pq->tree);

return CONTAINER_OF(n, struct k_thread, base.qnode_rb);
if (n != NULL) {
t = CONTAINER_OF(n, struct k_thread, base.qnode_rb);
}
return t;
}

#ifdef CONFIG_SCHED_MULTIQ
Expand Down Expand Up @@ -683,10 +691,14 @@ struct k_thread *_priq_mq_best(struct _priq_mq *pq)
return NULL;
}

struct k_thread *t = NULL;
sys_dlist_t *l = &pq->queues[__builtin_ctz(pq->bitmask)];
sys_dnode_t *n = sys_dlist_peek_head(l);

return CONTAINER_OF(n, struct k_thread, base.qnode_dlist);
if (n != NULL) {
t = CONTAINER_OF(n, struct k_thread, base.qnode_dlist);
}
return t;
}

int _unpend_all(_wait_q_t *wait_q)
Expand Down Expand Up @@ -884,7 +896,7 @@ void _impl_k_wakeup(k_tid_t thread)
return;
}

if (_abort_thread_timeout(thread) == _INACTIVE) {
if (_abort_thread_timeout(thread) < 0) {
irq_unlock(key);
return;
}
Expand Down
20 changes: 8 additions & 12 deletions kernel/timeout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,11 @@ static struct _timeout *next(struct _timeout *t)

static void remove_timeout(struct _timeout *t)
{
if (t->node.next != NULL && t->node.prev != NULL) {
if (next(t) != NULL) {
next(t)->dticks += t->dticks;
}

sys_dlist_remove(&t->node);
if (next(t) != NULL) {
next(t)->dticks += t->dticks;
}
t->node.next = t->node.prev = NULL;
t->dticks = _INACTIVE;

sys_dlist_remove(&t->node);
}

static s32_t elapsed(void)
Expand All @@ -64,7 +60,7 @@ static s32_t elapsed(void)

void _add_timeout(struct _timeout *to, _timeout_func_t fn, s32_t ticks)
{
__ASSERT(to->dticks < 0, "");
__ASSERT(!sys_dnode_is_linked(&to->node), "");
to->fn = fn;
ticks = max(1, ticks);

Expand Down Expand Up @@ -96,10 +92,10 @@ void _add_timeout(struct _timeout *to, _timeout_func_t fn, s32_t ticks)

int _abort_timeout(struct _timeout *to)
{
int ret = _INACTIVE;
int ret = -EINVAL;
Copy link
Contributor

@andyross andyross Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned elsewhere, probably best to just eliminate the return value here or make it a boolean to indicate "was active", as that's all existing callers care about.

Copy link
Contributor Author

@pabigot pabigot Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return indicates the attempt to abort the timeout failed because the timeout wasn't active (valid for abort). I believe this is the preferred way to indicate the result of the operation.


LOCKED(&timeout_lock) {
if (to->dticks != _INACTIVE) {
if (sys_dnode_is_linked(&to->node)) {
remove_timeout(to);
ret = 0;
}
Expand All @@ -112,7 +108,7 @@ s32_t z_timeout_remaining(struct _timeout *timeout)
{
s32_t ticks = 0;

if (timeout->dticks == _INACTIVE) {
if (_is_inactive_timeout(timeout)) {
return 0;
}

Expand Down
4 changes: 2 additions & 2 deletions kernel/timer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ Z_SYSCALL_HANDLER(k_timer_start, timer, duration_p, period_p)
void _impl_k_timer_stop(struct k_timer *timer)
{
unsigned int key = irq_lock();
bool inactive = (_abort_timeout(&timer->timeout) == _INACTIVE);
int inactive = _abort_timeout(&timer->timeout) != 0;

irq_unlock(key);

Expand Down Expand Up @@ -203,7 +203,7 @@ u32_t _impl_k_timer_status_sync(struct k_timer *timer)
u32_t result = timer->status;

if (result == 0) {
if (timer->timeout.dticks != _INACTIVE) {
if (!_is_inactive_timeout(&timer->timeout)) {
/* wait for timer to expire or stop */
(void)_pend_current_thread(key, &timer->wait_q, K_FOREVER);

Expand Down
4 changes: 4 additions & 0 deletions tests/kernel/common/src/dlist.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,13 @@ void test_dlist(void)
"test_list head/tail are wrong");

/* Finding and removing node 1 */
zassert_true(sys_dnode_is_linked(&test_node_1.node),
"node1 is not linked");
sys_dlist_remove(&test_node_1.node);
zassert_true((verify_emptyness(&test_list)),
"test_list should be empty");
zassert_false(sys_dnode_is_linked(&test_node_1.node),
"node1 is still linked");

/* Prepending node 1 */
sys_dlist_prepend(&test_list, &test_node_1.node);
Expand Down