Kernel runtime error checking #16702

nashif · 2019-06-07T21:24:55Z

Introduce runtime error checking in the kernel where it make sense and remove duplication of asserts and oopses that were used to catch such errors. Propagate errors where needed.

Ideally we want to use this to check for parameters and permissions and return to the caller to allow recovery, which was not possible in the current approach: when asserts are enabled (not default) we would just crash on every error, even if recoverable or we will get undefined behavior if asserts were not enabled.

Situations which should never happen and disallowed by design, such as running something in an ISR when it should not, remain as asserts where applicable.

(Edit by @aescolar) Discussed in #17105

zephyrbot · 2019-06-07T21:31:37Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

include/kernel.h

kernel/poll.c

pfl · 2019-06-26T22:35:44Z

kernel/queue.c

Nitipick: if the list is empty, the function could just return here?

andrewboie

You can get rid of some of the checks in sys_mutex code since the base implementation is now doing it.

kernel/mem_slab.c

kernel/msg_q.c

kernel/mutex.c

ceolin · 2019-09-25T21:56:02Z

kernel/pipes.c

can you make it boolean operations ? e.g z_waitq_head(&pipe->wait_q.readers) != NULL

ceolin · 2019-09-25T21:57:47Z

include/kernel.h

why change it if this function always return 0 ? There is nothing to check here.

aescolar

so, you are not approving?

I'm not the kernel maintainer, so it is for me to give a +2.
My concerns have been addressed. So this is just a +0.

ioannisg

some minor comments from my side

ioannisg · 2019-12-03T12:56:31Z

arch/Kconfig

Maybe some text in the help-text should be added, to justify the dependency

ioannisg · 2019-12-03T13:12:16Z

kernel/mem_slab.c

early return here, instead?

+0

andrewboie · 2019-12-03T15:58:10Z

My concerns have been addressed. So this is just a +0.

@aescolar this isn't helpful.
You obviously have strong opinions about this topic, you are allowed to give a +1

aescolar · 2019-12-03T16:02:23Z

you are allowed to give a +1

@andrewboie Thanks. I understand I'm allowed to approve the PR. But the thing is that I do not support the overall direction things are going, but I will not oppose it either while the inconvenience for my company or others trying to target constrained platforms are not too great.

ioannisg · 2019-12-03T16:10:38Z

you are allowed to give a +1

@andrewboie Thanks. I understand I'm allowed to approve the PR. But the thing is that I do not support the overall direction things are going, but I will not oppose it either while the inconvenience for my company or others trying to target constrained platforms are not too great.

@aescolar @nashif as far I understood, it is possible for this patch to have no overhead, if the introduced Kconfig switches are set properly, i.e. disable both ASSERT and NO_RUNTIME error checking, am I correct?

aescolar · 2019-12-03T16:30:59Z

@ioannisg My concern is two fold:

My approval of this PR may be miss-represented as support of further changes. And it is not clear to me what will happen after.
Already @nashif is implying that testing with anything else than RUNTIME_ERROR_CHECKS will not be done.

andrewboie · 2019-12-03T16:33:39Z

But the thing is that I do not support the overall direction things are going

What? Be specific

aescolar · 2019-12-03T16:35:36Z

What? Be specific

Disregard for platform constrained targets.
Over-prioritization of some assumed to be requirements of functional safety at the expense of any other target for the project.

nashif · 2019-12-03T16:40:36Z

Over-prioritization of some assumed to be requirements of functional safety at the expense of any other target for the project.

This has nothing to do with functional safety. The fact that we are not able to test basic APIs because we just crash is bad for EVERYONE.

andrewboie · 2019-12-03T16:42:16Z

Disregard for platform constrained targets.

@aescolar let's explore this.
How does this PR disregard platform constrained targets?
Be specific.

aescolar · 2019-12-03T16:48:02Z

@aescolar let's explore this.
How does this PR disregard platform constrained targets?

The fact that I removed my -1 would be an indication that it does not anymore. Wouldn't it?

addressed

Define there options for runtime error handling: - assert on all errors (ASSERT_ON_ERRORS) - no runtime checks (no asserts, no runtime error handling) (NO_RUNTIME_CHECKS) - full runtime error handling (the default) (RUNTIME_ERROR_CHECKS) Signed-off-by: Anas Nashif <[email protected]>

k_mutex_unlock will now perform error checking and return on failures. If the current thread does not own the mutex, we will now return -EPERM. In the unlikely situation where we own a lock and the lock count is zero, we assert. This is considered an undefined bahviour and should not happen. Signed-off-by: Anas Nashif <[email protected]>

Check for errors at runtime and stop depending on ASSERTs. This changes the API for - k_sem_init k_sem_init now returns -EINVAL on invalid data. Signed-off-by: Anas Nashif <[email protected]>

Remove static helper functions used only once and integrate them into calling functions. In k_sem_take, return at the end. Signed-off-by: Anas Nashif <[email protected]>

Add runtime error checking to k_pipe_cleanup and k_pipe_get and remove asserts. Adapted test which was expecting a fault to handle errors instead. Signed-off-by: Anas Nashif <[email protected]>

When trying to cancel a NULL work queue return -EAGAIN. Signed-off-by: Anas Nashif <[email protected]>

Add runtime error checking for k_mem_slab_init and replace asserts with returning error codes. Signed-off-by: Anas Nashif <[email protected]>

Add runtime error handling for k_msgq_cleanup. We return 0 on success now and -EAGAIN when cleanup is not possible. Signed-off-by: Anas Nashif <[email protected]>

Add runtime error checking for both k_stack_push and k_stack_cleanup. Signed-off-by: Anas Nashif <[email protected]>

Runtime error handling for k_queue_append_list and k_queue_merge_slist. Signed-off-by: Anas Nashif <[email protected]>

Add tests for k_sem_init. Signed-off-by: Anas Nashif <[email protected]>

nashif requested review from andrewboie and andyross as code owners June 7, 2019 21:24

nashif added the DNM This PR should not be merged (Do Not Merge) label Jun 7, 2019

zephyrbot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test area: Kernel labels Jun 7, 2019

nashif force-pushed the mutex_cleanup branch from ff2feb9 to 52a43ba Compare June 8, 2019 13:25

nashif force-pushed the mutex_cleanup branch 2 times, most recently from e67ea4e to 334a863 Compare June 17, 2019 19:29

nashif removed the DNM This PR should not be merged (Do Not Merge) label Jun 18, 2019

nashif requested review from MaureenHelm, carlescufi, ceolin, ioannisg and pfl June 26, 2019 19:58

ceolin reviewed Jun 26, 2019

View reviewed changes

include/kernel.h Outdated Show resolved Hide resolved

pfl approved these changes Jun 26, 2019

View reviewed changes

nashif force-pushed the mutex_cleanup branch from 334a863 to 35af094 Compare July 3, 2019 10:47

andrewboie suggested changes Jul 3, 2019

View reviewed changes

kernel/mem_slab.c Outdated Show resolved Hide resolved

kernel/msg_q.c Outdated Show resolved Hide resolved

kernel/mutex.c Outdated Show resolved Hide resolved

nashif force-pushed the mutex_cleanup branch 2 times, most recently from be503b3 to 492441d Compare July 8, 2019 14:37

nashif requested a review from andrewboie July 9, 2019 14:22

nashif force-pushed the mutex_cleanup branch from 492441d to e563673 Compare July 18, 2019 15:16

andrewboie mentioned this pull request Jul 23, 2019

abolish Z_OOPS() in system call handlers #17735

Closed

nashif force-pushed the mutex_cleanup branch from e563673 to a037c1e Compare August 28, 2019 13:27

nashif force-pushed the mutex_cleanup branch from a037c1e to 0b68b7b Compare September 24, 2019 15:10

dcpleung reviewed Sep 25, 2019

View reviewed changes

kernel/mutex.c Outdated Show resolved Hide resolved

ceolin reviewed Sep 25, 2019

View reviewed changes

kernel/pipes.c Outdated

Copy link

Member

ceolin Sep 25, 2019

Choose a reason for hiding this comment

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

can you make it boolean operations ? e.g z_waitq_head(&pipe->wait_q.readers) != NULL

ceolin reviewed Sep 25, 2019

View reviewed changes

include/kernel.h Outdated

Copy link

Member

ceolin Sep 25, 2019

Choose a reason for hiding this comment

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

why change it if this function always return 0 ? There is nothing to check here.

aescolar previously approved these changes Dec 3, 2019

View reviewed changes

ioannisg reviewed Dec 3, 2019

View reviewed changes

nashif force-pushed the mutex_cleanup branch from 12bce58 to f6dca7c Compare January 9, 2020 19:55

nashif force-pushed the mutex_cleanup branch 3 times, most recently from 2a4b2ac to 4db0fdf Compare January 16, 2020 16:35

nashif added 11 commits January 20, 2020 10:38

kernel: semaphore: k_sem_init error checking

536b325

Check for errors at runtime and stop depending on ASSERTs. This changes the API for - k_sem_init k_sem_init now returns -EINVAL on invalid data. Signed-off-by: Anas Nashif <[email protected]>

kernel: semaphore: optimize code

4847317

Remove static helper functions used only once and integrate them into calling functions. In k_sem_take, return at the end. Signed-off-by: Anas Nashif <[email protected]>

kernel: pipe: runtime error checking

dc97b44

Add runtime error checking to k_pipe_cleanup and k_pipe_get and remove asserts. Adapted test which was expecting a fault to handle errors instead. Signed-off-by: Anas Nashif <[email protected]>

kernel: work_q: error handling

736ad5b

When trying to cancel a NULL work queue return -EAGAIN. Signed-off-by: Anas Nashif <[email protected]>

kernel: mem_slab: error handling

490026f

Add runtime error checking for k_mem_slab_init and replace asserts with returning error codes. Signed-off-by: Anas Nashif <[email protected]>

kernel: msgq: error handling

58eda16

Add runtime error handling for k_msgq_cleanup. We return 0 on success now and -EAGAIN when cleanup is not possible. Signed-off-by: Anas Nashif <[email protected]>

kernel: stack: error handling

7e242ac

Add runtime error checking for both k_stack_push and k_stack_cleanup. Signed-off-by: Anas Nashif <[email protected]>

kernel: queue: runtime error handling

d2a8faf

Runtime error handling for k_queue_append_list and k_queue_merge_slist. Signed-off-by: Anas Nashif <[email protected]>

tests: test k_sem_init()

794d2be

Add tests for k_sem_init. Signed-off-by: Anas Nashif <[email protected]>

nashif force-pushed the mutex_cleanup branch from 4db0fdf to 794d2be Compare January 20, 2020 15:48

nashif merged commit 2711c4e into zephyrproject-rtos:master Jan 20, 2020

Kernel runtime error checking #16702

Kernel runtime error checking #16702

Uh oh!

Conversation

nashif commented Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zephyrbot commented Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pfl Jun 26, 2019

Choose a reason for hiding this comment

Uh oh!

andrewboie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ceolin Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

ceolin Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

aescolar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ioannisg left a comment

Choose a reason for hiding this comment

Uh oh!

ioannisg Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

ioannisg Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

andrewboie commented Dec 3, 2019

Uh oh!

aescolar commented Dec 3, 2019

Uh oh!

ioannisg commented Dec 3, 2019

Uh oh!

aescolar commented Dec 3, 2019

Uh oh!

andrewboie commented Dec 3, 2019

Uh oh!

aescolar commented Dec 3, 2019

Uh oh!

nashif commented Dec 3, 2019

Uh oh!

andrewboie commented Dec 3, 2019

Uh oh!

aescolar commented Dec 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

nashif commented Jun 7, 2019 •

edited

Loading

zephyrbot commented Jun 7, 2019 •

edited

Loading

aescolar left a comment •

edited

Loading