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

cpu/native: fix race in thread_yield_higher() [backport 2019.01] #10921

Merged
merged 2 commits into from
Feb 1, 2019
Merged

cpu/native: fix race in thread_yield_higher() [backport 2019.01] #10921

merged 2 commits into from
Feb 1, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Feb 1, 2019

Backport of #10891

Contribution description

Error case:
1. thread_yield_higher() stores the thread's ucontext
2. creates an "isr ucontext" for isr_thread_yield, switches to it

Case 1: no signals are pending, continues in isr_thread_yield()
3a. sched_run is called
4a. return to sched_active_thread ucontext

Case 2: signals pending (the crashing scenario), continues in native_irq_handler()
3b. handles signals
4b. if sched_context_switch_request is set, call sched_run
5b. return to sched_active_thread ucontext

4b misses the call to sched_run(), leading to a possible return into a
non-ready thread.

Fixes #10881.

Testing procedure

See #10881 and #10908

Issues/PRs references

Fixes #10881.
Fixes #6123.

@kaspar030 @miri64
cpu/native: fix race condition in thread_yield_higher()
84a5c4f 
Error case:
1. thread_yield_higher() stores the thread's ucontext
2. creates an "isr ucontext" for isr_thread_yield, switches to it

Case 1: no signals are pending, continues in isr_thread_yield()
3a. sched_run is called
4a. return to sched_active_thread ucontext

Case 2: signals pending (the crashing scenario), continues in native_irq_handler()
3b. handles signals
4b. if sched_context_switch_request is set, call sched_run
5b. return to sched_active_thread ucontext

4b misses the call to sched_run(), leading to a possible return into a
non-ready thread.

(cherry picked from commit 62bb4cc)
@kaspar030 @miri64
core/msg: add assert for problem identified in RIOT-OS#10881
b3ede8d 
(cherry picked from commit 82b2362)
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Platform: native Platform: This PR/issue effects the native platform Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Feb 1, 2019
@miri64 miri64 requested a review from kaspar030 February 1, 2019 15:23
@miri64 miri64 mentioned this pull request Feb 1, 2019
Merged
kaspar030
kaspar030 approved these changes Feb 1, 2019
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@miri64
Copy link
Member Author

miri64 commented Feb 1, 2019

I don't believe this is a sensible ACK 😆

@kaspar030
Copy link
Contributor

I don't believe this is a sensible ACK laughing

Not all ACKs are created equal? 😉

Seriously, the code it touches hasn't been changed in a long while. The change is as valid as in master. If it compiles and finishes the CI-tests, what could possibly go wrong?

@miri64
Copy link
Member Author

miri64 commented Feb 1, 2019

what could possibly go wrong?

Famous last words ;-P

@miri64
Copy link
Member Author

miri64 commented Feb 1, 2019

But on a more somber note: I prefer if a person that is not us two would have a look before we merge this into the stable branch.

emmanuelsearch
emmanuelsearch approved these changes Feb 1, 2019
View reviewed changes
Copy link
Member

@emmanuelsearch emmanuelsearch left a comment •
edited
Loading

Choose a reason for hiding this comment

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

clean backport ;)

@emmanuelsearch emmanuelsearch merged commit 1a4d083 into RIOT-OS:2019.01-branch Feb 1, 2019
@miri64 miri64 deleted the backport/2019.01/fix_native_thread_yield_higher branch February 1, 2019 16:08
@aabadie aabadie added this to the Release 2019.01 milestone Feb 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaspar030 kaspar030 kaspar030 approved these changes

@emmanuelsearch emmanuelsearch emmanuelsearch approved these changes

@aabadie aabadie Awaiting requested review from aabadie

Assignees
No one assigned
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Platform: native Platform: This PR/issue effects the native platform Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.01
Development

Successfully merging this pull request may close these issues.
4 participants
@miri64 @kaspar030 @emmanuelsearch @aabadie