!threads: implement robust and priority ceiling locks#815
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for robust mutexes and the priority ceiling protocol, adding new syscalls and internal functions such as mutexConsistent and mutexPrioCeiling. Feedback highlights several critical issues: priority is not immediately elevated to the ceiling upon lock acquisition, proc_lockConsistent lacks robust-lock validation and allows non-owners to clear inconsistency, proc_mutexCreate does not validate the protocol field, proc_lockPrioCeiling fails to check for invalid negative ceiling values, and disabling assertions with #if 0 leaves dead code in the codebase.
| else if (prioceiling > MAX_PRIO) { | ||
| err = -EINVAL; | ||
| } |
There was a problem hiding this comment.
proc_lockPrioCeiling does not check if prioceiling < -1. It should return -EINVAL for any negative value other than -1 (which is used to retrieve the ceiling).
else if (prioceiling < -1 || prioceiling > MAX_PRIO) {
err = -EINVAL;
}| static int _proc_lockClear(lock_t *lock) | ||
| { | ||
| #ifndef NDEBUG | ||
| #if 0 |
There was a problem hiding this comment.
Changing #ifndef NDEBUG to #if 0 disables assertions completely and leaves dead code in the codebase. If the ownership assertion is invalid for PH_LOCK_NORMAL locks (which can be unlocked by other threads), the assertion should be conditionally checked based on the lock type instead of being completely disabled.
#ifndef NDEBUG![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return -EINVAL; | ||
| } | ||
|
|
||
| if (attr->protocol == PH_LOCK_PROTO_PRIOCEILING && attr->prioceiling > MAX_PRIO) { |
| return -EINVAL; | ||
| } | ||
|
|
||
| if (attr->protocol == PH_LOCK_PROTO_PRIOCEILING && attr->prioceiling > MAX_PRIO) { |
| _Static_assert(sizeof(threads_common.ready) / sizeof(threads_common.ready[0]) <= (u8)-1, "queue size must fit into priority type"); | ||
|
|
||
| #define MAX_PRIO ((u8)(sizeof(threads_common.ready) / sizeof(threads_common.ready[0])) - 1U) | ||
| _Static_assert(MAX_PRIO <= (u8)-1, "MAX_PRIO must fit into priority type"); |
| thread_t *thread = lock->queue; | ||
|
|
||
| if (thread != NULL) { | ||
| if (lock->attr.protocol == PH_LOCK_PROTO_PRIOCEILING) { |
| return lock->attr.prioceiling; | ||
| } | ||
|
|
||
| if (lock->attr.protocol == PH_LOCK_PROTO_INHERIT && thread != NULL) { |
|
|
||
| lock->owner = current; | ||
| lock->depth = 1; | ||
| if (lock->inconsistent != 0 && lock->attr.robust != 0) { |
| hal_spinlockSet(&lock->spinlock, &sc); | ||
|
|
||
| if (lock->owner == NULL || lock->owner == proc_current()) { | ||
| if (lock->inconsistent != 0) { |
|
|
||
| hal_spinlockSet(&lock->spinlock, &sc); | ||
|
|
||
| if (lock->attr.protocol != PH_LOCK_PROTO_PRIOCEILING) { |
| err = -EINVAL; | ||
| } | ||
| else { | ||
| err = lock->attr.prioceiling; |
| err = lock->attr.prioceiling; | ||
| if (prioceiling >= 0) { | ||
| if (lock->owner == NULL || lock->owner == proc_current()) { | ||
| lock->attr.prioceiling = prioceiling; |
TASK: RTOS-1399
Some assertions are too radical for the DEBUG=1. The panic on "unlock on unlocked lock" assertions is particularly too harsh, as the kernel can easily recover from such state and return -EPERM. POSIX tests will test that path and since the CI test runner runs under DEBUG=1, the test would fail. TASK: RTOS-1399
99aa11d to
324eb5b
Compare
| return -EINVAL; | ||
| } | ||
|
|
||
| if ((attr->protocol != PH_LOCK_PROTO_INHERIT) && (attr->protocol != PH_LOCK_PROTO_NOINHERIT) && (attr->protocol != PH_LOCK_PROTO_PRIOCEILING)) { |
| return -EINVAL; | ||
| } | ||
|
|
||
| if ((attr->protocol != PH_LOCK_PROTO_INHERIT) && (attr->protocol != PH_LOCK_PROTO_NOINHERIT) && (attr->protocol != PH_LOCK_PROTO_PRIOCEILING)) { |
| return -EINVAL; | ||
| } | ||
|
|
||
| if ((attr->protocol != PH_LOCK_PROTO_INHERIT) && (attr->protocol != PH_LOCK_PROTO_NOINHERIT) && (attr->protocol != PH_LOCK_PROTO_PRIOCEILING)) { |
| hal_spinlockSet(&lock->spinlock, &sc); | ||
|
|
||
| if (lock->owner == proc_current()) { | ||
| if (lock->attr.robust != 0 && lock->inconsistent != 0) { |
|
|
||
| hal_spinlockSet(&lock->spinlock, &sc); | ||
|
|
||
| if (lock->attr.protocol != PH_LOCK_PROTO_PRIOCEILING) { |
2 participants
TASK: RTOS-1399
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment