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.

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

core/mbox: fix race condition [backport 2022.10] #18960

Merged

Conversation

benpicco
Copy link
Contributor

Backport of #18955

Contribution description

The mbox code contains a race condition in mbox_put(): When it waits for a slot in the queue to become available, it is woken up with IRQs enabled. It disables IRQs again as first thing, but by then another thread may already have preempted the running thread and filled the queue back up. In this case, a message in the queue would be silently overwritten.

Testing procedure

Actually triggering the race condition would require some effort with a specially crafted test application, which I am unable to currently provide. It should however be quite easy to reason that the change fixes the potential race condition.

In addition, the usual regression tests may make sense.

Issues/PRs references

None

@benpicco benpicco requested a review from kaspar030 as a code owner November 23, 2022 14:21
@benpicco benpicco added Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR 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 Nov 23, 2022
@benpicco benpicco requested a review from maribu November 23, 2022 14:21
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.

backport ACK.

@kaspar030 kaspar030 enabled auto-merge November 23, 2022 15:10
@maribu
Copy link
Member

maribu commented Nov 23, 2022

#18959 is in front of the CI queue, so let's wait for that to be merged

@maribu maribu removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 23, 2022
The mbox code contains a race condition in `mbox_put()`: When it
waits for a slot in the queue to become available, it is woken up with
IRQs enabled. It disables IRQs again as first thing, but by then
another thread may already have preempted the running thread and filled
the queue back up. In this case, a message in the queue would be
silently overwritten.

(cherry picked from commit 42b9334)
@maribu maribu force-pushed the backport/2022.10/core/mbox/fix-race branch from 75e34b3 to 231b1a1 Compare November 23, 2022 16:36
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 23, 2022
@maribu
Copy link
Member

maribu commented Nov 23, 2022

#18959 is in front of the CI queue, so let's wait for that to be merged

It's in, this PR is rebased and back into the CI queue :)

@riot-ci
Copy link

riot-ci commented Nov 23, 2022

Murdock results

✔️ PASSED

231b1a1 core/mbox: fix race condition

Success Failures Total Runtime
115836 0 115836 01h:56m:11s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@kaspar030 kaspar030 merged commit 68f7378 into RIOT-OS:2022.10-branch Nov 23, 2022
@benpicco benpicco deleted the backport/2022.10/core/mbox/fix-race branch November 23, 2022 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR 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
Development

Successfully merging this pull request may close these issues.

4 participants