Skip to content
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

Jump to bottom

When the delay parameter of k_delayed_work_submit is K_FOREVER, the system will crash #24409

Closed
LuoZhongYao opened this issue Apr 16, 2020 · 2 comments · Fixed by #24565
Closed

When the delay parameter of k_delayed_work_submit is K_FOREVER, the system will crash #24409

LuoZhongYao opened this issue Apr 16, 2020 · 2 comments · Fixed by #24565
Assignees
@andyross @nordic-krch
Labels
area: Kernel bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@LuoZhongYao
Copy link
Contributor

Describe the bug
If the k_delayed_work_submit function is called with the K_FOREVER parameter, the system will error

To Reproduce

  1. Use any board
  2. Call the k_delayed_work_submit function and set the parameter to K_FOREVER, then call k_delayed_work_submit through another k_delayed_work

Expected behavior
Calling k_delayed_work_submit with K_FOREVER can directly return an error, or directly return..

Environment (please complete the following information):

  • OS: Any
  • Toolchain Zephyr SDK
  • Commit SHA 40ee38a
@LuoZhongYao LuoZhongYao added the bug The issue is a bug, or the PR is fixing a bug label Apr 16, 2020
@carlescufi
Copy link
Member

@andyross we are likely missing a test for this particular usage of delayed work?

@carlescufi carlescufi added area: Kernel priority: medium Medium impact/importance bug labels Apr 21, 2020
@andyross
Copy link
Contributor

This actually never worked correctly (well, "correctly" -- it's a pathological case after all), and AFAICT the behavior didn't change with the timeout patch. What actually happens in practice is that the delayed work timeout gets set for -1 (== K_FOREVER) ticks into the future, which means it fires immediately at the next timer interrupt.

I can't reproduce a crash, btw; it just fires normally but unexpectedly. Maybe your app is hitting an unexpected situation and crashing?

As far as a fix: we can either document this as a "don't do that", or (what I think I'll submit) add a check to z_add_timeout() which noops if a K_FOREVER reaches down that far.

andyross pushed a commit to andyross/zephyr that referenced this issue Apr 21, 2020
kernel/timeout: Check for K_FOREVER in z_add_timeout()
a6aa4e1 
The "forever" token has always been interpreted above z_add_timeout()
(because it's always taken ticks, but K_FOREVER used to be in ms).
But it was discovered that k_delayed_work_submit_to_queue() was never
testing for this and passing a raw K_FOREVER down, where it got
interpreted as a negative timeout and caused it to fire at the next
tick.

Now that we actually see the original k_timeout_t here, we might as
well check it locally and do the correct thing (that is, nothing) if
asked to schedule a timeout that will never fire.

Fixes zephyrproject-rtos#24409

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
@andyross andyross mentioned this issue Apr 21, 2020
Merged
andrewboie pushed a commit that referenced this issue Apr 22, 2020
@andrewboie
kernel/timeout: Check for K_FOREVER in z_add_timeout()
12bd187 
The "forever" token has always been interpreted above z_add_timeout()
(because it's always taken ticks, but K_FOREVER used to be in ms).
But it was discovered that k_delayed_work_submit_to_queue() was never
testing for this and passing a raw K_FOREVER down, where it got
interpreted as a negative timeout and caused it to fire at the next
tick.

Now that we actually see the original k_timeout_t here, we might as
well check it locally and do the correct thing (that is, nothing) if
asked to schedule a timeout that will never fire.

Fixes #24409

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
sandeepbrcm pushed a commit to Broadcom/zephyr that referenced this issue Apr 30, 2020
kernel/timeout: Check for K_FOREVER in z_add_timeout()
8173c72 
The "forever" token has always been interpreted above z_add_timeout()
(because it's always taken ticks, but K_FOREVER used to be in ms).
But it was discovered that k_delayed_work_submit_to_queue() was never
testing for this and passing a raw K_FOREVER down, where it got
interpreted as a negative timeout and caused it to fire at the next
tick.

Now that we actually see the original k_timeout_t here, we might as
well check it locally and do the correct thing (that is, nothing) if
asked to schedule a timeout that will never fire.

Fixes zephyrproject-rtos#24409

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
hakehuang pushed a commit to hakehuang/zephyr that referenced this issue Jun 20, 2020
@hakehuang
kernel/timeout: Check for K_FOREVER in z_add_timeout()
b7bc830 
The "forever" token has always been interpreted above z_add_timeout()
(because it's always taken ticks, but K_FOREVER used to be in ms).
But it was discovered that k_delayed_work_submit_to_queue() was never
testing for this and passing a raw K_FOREVER down, where it got
interpreted as a negative timeout and caused it to fire at the next
tick.

Now that we actually see the original k_timeout_t here, we might as
well check it locally and do the correct thing (that is, nothing) if
asked to schedule a timeout that will never fire.

Fixes zephyrproject-rtos#24409

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@andyross andyross

@nordic-krch nordic-krch

Labels
area: Kernel bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

kernel/timeout: Check for K_FOREVER in z_add_timeout() andyross/zephyr
4 participants
@andyross @LuoZhongYao @carlescufi @nordic-krch