From 217e9f8f93588423530b5bdad5b4329c14e1e624 Mon Sep 17 00:00:00 2001 From: Furqan Munir Date: Mon, 31 Aug 2026 03:32:54 +0500 Subject: [PATCH] =?UTF-8?q?=EF=BB=BFfix(kernel):=20restore=20transitive=20?= =?UTF-8?q?priority=20inheritance=20and=20green=20build?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 02c490a ("restore mutex owner priority after waiter timeout") left the kernel unable to compile and silently dropped transitive priority inheritance: - kernel/src/sync.c referenced m->original_prio (no such field in mtx_t), task_valid(), g_blocked_on, and pi_propagate() - all of which the commit removed, producing 9 compile errors. - kernel/src/task.c gained a duplicate eos_task_set_current_internal definition - a further compile error. - tests/test_kernel.c silently stopped running test_task_stats() and test_tick_overflow(), reporting "10/10" while 12 tests exist. - CHANGELOG.md described a fix that never landed and clobbered the device-tree parser entries. Restore the transitive priority-inheritance machinery from 9a61678 (pi_recompute/pi_propagate over a g_blocked_on chain), remove the duplicate definition, re-register the dropped tests, and repair the changelog. Verified with zig cc (clang): kernel sources compile clean under -Wall -Wextra; test_priority_inherit passes 6/6 and test_kernel passes 12/12. --- CHANGELOG.md | 9 +++- TASKS.md | 1 + kernel/src/sync.c | 100 +++++++++++++++++++++++++++++++------------- kernel/src/task.c | 6 --- tests/test_kernel.c | 4 +- 5 files changed, 84 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1cea9107..e088a7b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,14 @@ - **`eos_queue_create`:** Size check now uses division so `item_size * capacity` cannot wrap `size_t` on 32-bit targets and overflow the 1024-byte queue store. - **`eos_sem_create`:** Reject `initial > max` and `max` values that do not fit in `int32_t`, so the counting-semaphore invariant cannot be created already broken. - **`eos_mutex_lock`:** Recursive lock returns `EOS_KERN_FULL` at `uint8_t` saturation instead of wrapping `rec_count` to 0 and leaving the mutex stuck. -- **`eos_mutex_lock`:** A waiter that times out no longer leaves the mutex owner permanently boosted. Owner priority is recomputed from remaining waiters (or restored to `original_prio`). A full waiter table now returns `EOS_KERN_NO_MEMORY` without applying a boost. +- **`eos_mutex_lock`:** A waiter that times out no longer leaves the mutex owner permanently boosted. The owner's effective priority is recomputed from its base priority and the remaining waiters, so the boost propagates transitively along the blocking chain and is withdrawn when a waiter leaves. A full waiter table returns `EOS_KERN_NO_MEMORY` without applying a boost. +- **`eos_dt_parse`:** Bounds-check the flattened device tree blob before dereferencing it. A malformed DTB could previously read outside the buffer four different ways: `off_struct` past the end of the blob (the `size - off_struct` bound wrapped), a node name with no terminator (unbounded `strlen`), a property name offset outside the strings block, and nesting deeper than the 32-entry node stack. All are now rejected. +- **`eos_dt_find_compatible`:** Search within `prop->len` instead of calling `strstr()` on a property value that is not guaranteed to be NUL-terminated. +- **`eos_dt_get_irq`:** Reject a negative index, and one large enough that `index * 4` overflows, before it is used as an offset. + +### Added +- `tests/test_devicetree.c` — device tree parser suite: happy path, malformed-blob rejection, a sweep over every truncation of a valid blob, and a single-byte corruption sweep. +- `tests/fuzz/` is now wired into the build. The directory was never added by any `add_subdirectory`, so no fuzz target could be built. `fuzz_devicetree` is enabled and calls the real `eos_dt_parse()` API — it previously declared a non-existent `eos_dtb_parse()` and was commented out. ## [3.0.1] - 2026-05-16 diff --git a/TASKS.md b/TASKS.md index abb8384b..84bfe532 100644 --- a/TASKS.md +++ b/TASKS.md @@ -24,6 +24,7 @@ Status is one of: `todo`, `in-progress`, `blocked`, `review`, `done`. | T-002 | Fail CI when no tests are found | backend | reviewer | Added `--no-tests=error`. Against the old broken config `ctest` now exits **8**; against the fixed config it exits **0**. Both verified. | | T-003 | Make the test build link | architect | reviewer | 7 of 19 targets failed to link (`test_power`, `test_multicore`, `test_net`, `test_ota`, `test_sensor`, `test_filesystem`, `test_motor_ctrl`) — module sources are wrapped in `#if EOS_ENABLE_` and `eos_config.h` defaults each to 0, so with no product profile they compiled to empty translation units. A test `#define`-ing the flag only affects its own TU. Flags now set before the first `add_library()`. Result: build exit 0, **19/19 tests passed**. | | T-004 | Replace unverifiable README claims | docs | reviewer | Static `Status-Production Ready`, `Build-Passing`, `Coverage-100%` badges replaced with real workflow badges. "100% test coverage" removed — there is no coverage instrumentation in `CMakeLists.txt`, so no figure can be produced. The Zephyr/FreeRTOS/Linux benchmark claim removed — no comparative benchmark exists in this tree. | +| T-009 | Restore kernel build broken by commit `02c490a` (PR #55) | backend | — | `kernel/src/sync.c` referenced `m->original_prio` (field absent from `mtx_t`), `task_valid`, `g_blocked_on`, `pi_propagate` (all removed) — 9 compile errors; `kernel/src/task.c` had a duplicate `eos_task_set_current_internal` — 1 error. Restored the transitive priority-inheritance machinery from `9a61678`, removed the duplicate definition, re-registered the two kernel tests the commit dropped (`test_task_stats`, `test_tick_overflow`), and repaired the clobbered CHANGELOG. Verified with `zig cc`: `sync.c`/`task.c` compile clean under `-Wall -Wextra`; `test_priority_inherit` 6/6 and `test_kernel` 12/12 pass. | --- diff --git a/kernel/src/sync.c b/kernel/src/sync.c index 2d9db1f1..14dd98c2 100644 --- a/kernel/src/sync.c +++ b/kernel/src/sync.c @@ -45,31 +45,78 @@ typedef struct { static mtx_t g_mtx[EOS_MAX_MUTEXES]; static sem_t g_sem[EOS_MAX_SEMAPHORES]; -static void mtx_remove_waiter(mtx_t *m, uint8_t task) +/* ============================================================ + * Priority inheritance + * + * A task holding mutexes runs at + * + * effective = min( base_priority, + * priority of every task waiting on every mutex it owns ) + * + * with lower meaning more urgent. Recomputing from that invariant on every + * change, instead of saving and restoring a value around each lock, keeps the + * nested cases correct and lets a boost propagate along the blocking chain. + * ============================================================ */ + +/* Mutex each task is blocked on, as handle + 1 so that 0 means "none". */ +static uint8_t g_blocked_on[EOS_MAX_TASKS]; + +static inline int task_valid(uint8_t t) { - for (int i = 0; i < m->waiter_count; i++) { - if (m->waiters[i] == task) { - for (int j = i; j < m->waiter_count - 1; j++) - m->waiters[j] = m->waiters[j + 1]; - m->waiter_count--; - break; + return t < EOS_MAX_TASKS; +} + +/** @return non-zero if the effective priority changed. */ +static int pi_recompute(uint8_t task) +{ + if (!task_valid(task)) return 0; + + uint8_t eff = g_tasks[task].base_priority; + + for (int i = 0; i < EOS_MAX_MUTEXES; i++) { + const mtx_t *m = &g_mtx[i]; + if (!m->in_use || !m->locked || m->owner != task) continue; + for (int w = 0; w < m->waiter_count; w++) { + uint8_t waiter = m->waiters[w]; + if (!task_valid(waiter)) continue; + if (g_tasks[waiter].priority < eff) + eff = g_tasks[waiter].priority; } } + + if (g_tasks[task].priority == eff) return 0; + g_tasks[task].priority = eff; + return 1; } -/* Recompute inherited priority from remaining waiters, or restore original. */ -static void mtx_recompute_owner_priority(mtx_t *m) +static void pi_propagate(uint8_t task) { - if (m->owner == 0xFF) - return; + for (int depth = 0; depth < PI_MAX_CHAIN_DEPTH; depth++) { + if (!task_valid(task)) return; + if (!pi_recompute(task) && depth > 0) return; - uint8_t prio = m->original_prio; + uint8_t enc = g_blocked_on[task]; + if (enc == 0) return; + + uint8_t m = (uint8_t)(enc - 1u); + if (m >= EOS_MAX_MUTEXES) return; + if (!g_mtx[m].in_use || !g_mtx[m].locked) return; + if (g_mtx[m].owner == task) return; + + task = g_mtx[m].owner; + } +} + +static int mtx_remove_waiter(mtx_t *m, uint8_t task) +{ for (int i = 0; i < m->waiter_count; i++) { - uint8_t wp = eos_task_get_priority_internal(m->waiters[i]); - if (wp < prio) - prio = wp; + if (m->waiters[i] != task) continue; + for (int j = i; j < m->waiter_count - 1; j++) + m->waiters[j] = m->waiters[j + 1]; + m->waiter_count--; + return 1; } - eos_task_set_priority_internal(m->owner, prio); + return 0; } int eos_mutex_create(eos_mutex_handle_t *out) @@ -123,21 +170,17 @@ int eos_mutex_lock(eos_mutex_handle_t h, uint32_t timeout_ms) return EOS_KERN_TIMEOUT; } - /* Waiter table full: fail without boosting, or the owner would stay - * elevated even though this caller is never queued. */ + /* Refuse rather than block: a task that is not enqueued is never granted + * the mutex, so with EOS_WAIT_FOREVER it would sleep forever. */ if (g_mtx[h].waiter_count >= MTX_MAX_WAITERS) { eos_port_exit_critical(crit); return EOS_KERN_NO_MEMORY; } - /* Priority inheritance: boost owner to caller's priority if higher */ - uint8_t caller_prio = eos_task_get_priority_internal(caller); - uint8_t owner_prio = eos_task_get_priority_internal(g_mtx[h].owner); - if (caller_prio < owner_prio) { - eos_task_set_priority_internal(g_mtx[h].owner, caller_prio); - } - g_mtx[h].waiters[g_mtx[h].waiter_count++] = caller; + if (task_valid(caller)) + g_blocked_on[caller] = (uint8_t)(h + 1u); + pi_propagate(g_mtx[h].owner); /* Block the calling task */ eos_task_block_with_timeout(caller, timeout_ms); @@ -154,10 +197,11 @@ int eos_mutex_lock(eos_mutex_handle_t h, uint32_t timeout_ms) return EOS_KERN_OK; } - /* Timeout — drop from the wait queue and drop the inheritance boost - * if this waiter was the reason the owner was running elevated. */ + /* Timeout — leave the queue and drop the boost we were causing. */ + uint8_t owner = g_mtx[h].owner; mtx_remove_waiter(&g_mtx[h], caller); - mtx_recompute_owner_priority(&g_mtx[h]); + pi_propagate(owner); + eos_port_exit_critical(crit); return EOS_KERN_TIMEOUT; } diff --git a/kernel/src/task.c b/kernel/src/task.c index 9ff04800..2a449b03 100644 --- a/kernel/src/task.c +++ b/kernel/src/task.c @@ -572,9 +572,3 @@ int eos_task_get_all_stats(eos_task_stats_t *out, int max_entries, int *count) *count = n; return EOS_KERN_OK; } - -void eos_task_set_current_internal(eos_task_handle_t h) -{ - if (h >= EOS_MAX_TASKS) return; - g_current = (int)h; -} diff --git a/tests/test_kernel.c b/tests/test_kernel.c index 4eaa1f57..25969a30 100644 --- a/tests/test_kernel.c +++ b/tests/test_kernel.c @@ -268,6 +268,8 @@ int main(void) { test_semaphore(); test_queue(); test_queue_full(); - printf("=== ALL KERNEL TESTS PASSED (10/10) ===\n"); + test_task_stats(); + test_tick_overflow(); + printf("=== ALL KERNEL TESTS PASSED (12/12) ===\n"); return 0; } \ No newline at end of file