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

Implementation of osKernelRestoreLock is dangerous and might leave the kernel locked #109

Closed
pakluba opened this issue Jul 22, 2024 · 2 comments
Closed

Implementation of osKernelRestoreLock is dangerous and might leave the kernel locked #109

pakluba opened this issue Jul 22, 2024 · 2 comments
Assignees
@VladimirUmek
Labels
bug

Comments

@pakluba
Copy link
Contributor

pakluba commented Jul 22, 2024 •
edited
Loading

CMSIS-FreeRTOS/CMSIS/RTOS2/FreeRTOS/Source/cmsis_os2.c

Line 397 in ba82e7e

case taskSCHEDULER_SUSPENDED:

Unlike RTX, FreeRTOS does nesting in vTaskSuspendAll(). If the scheduling has already been suspended before osKernelLock() is called, osKernelRestoreLock() will increase the nesting level by 1 instead of removing 1 lock. So:

vTaskSuspendAll();           // the nesting level is at least 1
...
state = osKernelLock();      // does not increase nesting, returns 1 (was locked already)
...
osKernelRestoreLock(state);  // will call vTaskSuspendAll() to "restore" the lock -> boom...
@VladimirUmek
Copy link
Collaborator

Hello,
good find, thanks! I'll review the implementation and provide a fix.

@VladimirUmek VladimirUmek self-assigned this Jul 22, 2024
pakluba added a commit to pakluba/CMSIS-FreeRTOS that referenced this issue Aug 9, 2024
@pakluba
Do not blindly restore lock state, but depend on the current state (A…
0ba331d 
…RM-software#109)

The scheduler state checking has been removed as the scheduler being locked out
could still be valid due to the nesting.
VladimirUmek pushed a commit that referenced this issue Aug 19, 2024
@pakluba @VladimirUmek
Do not blindly restore lock state, but depend on the current state (#109
533ca44 
)

The scheduler state checking has been removed as the scheduler being locked out
could still be valid due to the nesting.
@VladimirUmek
Copy link
Collaborator

Thanks you for going forward and providing the PR! It has been merged hence I'm closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@VladimirUmek VladimirUmek

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@VladimirUmek @pakluba