patrick/zephyr
1
0
Fork 0
Code Issues Pull requests Actions 7 Packages Projects Releases Wiki Activity

subsys/cmsis_v*: Fix racy mutex testing, decouple from k_mutex

Browse source 
The cmsis implementations of osMutex was trying to inspect internal
k_mutex state (the owner and lock count) in the process of trying to
acquire the lock.  This is unfixably racy, by definition other
contexts will be trying to do the same on the unsynchronized data.

As far as I can tell, the only purpose was to be able to synthesize
osMutex's specified error behavior, which we can do with the existing
return codes from k_mutex_lock().  Add similar logic to osSemaphore,
which didn't have the race but was likewise abusing access to kernel
internals.

Signed-off-by: Andy Ross <andyross@google.com>
This commit is contained in:
Andy Ross 2022-10-12 11:55:13 -07:00 committed by Carles Cufí
parent 13f78fb005
commit 45ea83af89
3 changed files with 12 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

13
subsys/portability/cmsis_rtos_v1/cmsis_mutex.c
View file

@ -61,12 +61,12 @@ osStatus osMutexWait(osMutexId mutex_id, uint32_t timeout)
status = k_mutex_lock(mutex, K_MSEC(timeout));
}
if (status == -EBUSY) {
return osErrorResource;
} else if (status == -EAGAIN) {
if (timeout != 0 && (status == -EAGAIN || status == -EBUSY)) {
return osErrorTimeoutResource;
} else {
} else if (status == 0) {
return osOK;
} else {
return osErrorResource;
}
}
@ -85,13 +85,10 @@ osStatus osMutexRelease(osMutexId mutex_id)
return osErrorISR;
}
/* Mutex was not obtained before or was not owned by current thread */
if ((mutex->lock_count == 0) || (mutex->owner != _current)) {
if (k_mutex_unlock(mutex) != 0) {
return osErrorResource;
}
k_mutex_unlock(mutex);
return osOK;
}

4
subsys/portability/cmsis_rtos_v1/cmsis_semaphore.c
View file

@ -69,8 +69,10 @@ int32_t osSemaphoreWait(osSemaphoreId semaphore_id, uint32_t timeout)
*/
if (status == 0) {
return k_sem_count_get(semaphore) + 1;
} else {
} else if (status == -EBUSY || status == -EAGAIN) {
return 0;
} else {
return -1;
}
}

26
subsys/portability/cmsis_rtos_v2/mutex.c
View file

@ -74,20 +74,6 @@ osStatus_t osMutexAcquire(osMutexId_t mutex_id, uint32_t timeout)
return osErrorISR;
}
if (mutex->z_mutex.lock_count == 0U ||
mutex->z_mutex.owner == _current) {
}
/* Throw an error if the mutex is not configured to be recursive and
* the current thread is trying to acquire the mutex again.
*/
if ((mutex->state & osMutexRecursive) == 0U) {
if ((mutex->z_mutex.owner == _current) &&
(mutex->z_mutex.lock_count != 0U)) {
return osErrorResource;
}
}
if (timeout == osWaitForever) {
status = k_mutex_lock(&mutex->z_mutex, K_FOREVER);
} else if (timeout == 0U) {
@ -97,10 +83,10 @@ osStatus_t osMutexAcquire(osMutexId_t mutex_id, uint32_t timeout)
K_TICKS(timeout));
}
if (status == -EBUSY) {
return osErrorResource;
} else if (status == -EAGAIN) {
if (timeout != 0 && (status == -EAGAIN || status == -EBUSY)) {
return osErrorTimeout;
} else if (status != 0) {
return osErrorResource;
} else {
return osOK;
}
@ -121,14 +107,10 @@ osStatus_t osMutexRelease(osMutexId_t mutex_id)
return osErrorISR;
}
/* Mutex was not obtained before or was not owned by current thread */
if ((mutex->z_mutex.lock_count == 0U) ||
(mutex->z_mutex.owner != _current)) {
if (k_mutex_unlock(&mutex->z_mutex) != 0) {
return osErrorResource;
}
k_mutex_unlock(&mutex->z_mutex);
return osOK;
}