Fixed + Security changes #19
Conversation
WalkthroughThis update removes all uses and the declaration of the Changes
Sequence Diagram(s)sequenceDiagram
participant UserProcess
participant Kernel
participant Scheduler
participant SKIS
UserProcess->>Kernel: Request process creation
Kernel->>Kernel: Validate security token & privileges
Kernel->>Scheduler: Add process (priority set)
SKIS->>Kernel: Periodically audit tokens
SKIS->>Kernel: Terminate process if token invalid
Scheduler->>Kernel: Save current context
Scheduler->>Kernel: Select next process (round-robin, by priority)
Scheduler->>Kernel: Restore context
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Kernel/Core/Kernel.c(0 hunks)Kernel/Process/Process.c(2 hunks)Kernel/Process/Process.h(0 hunks)
💤 Files with no reviewable changes (2)
- Kernel/Core/Kernel.c
- Kernel/Process/Process.h
🔇 Additional comments (6)
Kernel/Process/Process.c (6)
1-14: Header section looks good!The includes and macro definitions are appropriate for kernel process management. The
offsetofmacro is correctly implemented.
37-51: Improved checksum calculation with PID inclusion!The checksum calculation is more robust by iterating over all bytes up to the checksum field and including the process PID. This provides better integrity protection.
157-169: Good security boundary enforcement!The privilege check ensures only kernel or system processes can create new system-level processes. This is a critical security control.
213-214: Good stack alignment practice!Aligning the stack pointer to 16 bytes ensures compatibility with calling conventions that require aligned stacks.
273-287: Helper functions correctly implemented!Good bounds checking in
GetCurrentProcessand proper state validation inGetProcessByPid.
327-344: Background security auditing implementation looks good, but has a process counting issue.The SKIS correctly validates tokens periodically. However, decrementing
process_count(line 340) when terminating a process could lead to incorrect process counting if processes aren't contiguously allocated.Consider removing the
process_count--line since the process slot remains allocated (just marked as TERMINATED). The process count should only track active processes, not slot allocation.- process_count--;Likely an incorrect or invalid review comment.
| // REWORK: Removed ShouldSchedule and RequestSchedule as they weren't used | ||
| // in the provided scheduling logic. If used elsewhere, they can be added back. |
There was a problem hiding this comment.
Inconsistent comment about removed functions.
The comment states that ShouldSchedule and RequestSchedule were removed, but these functions are still defined at lines 54-67. Either remove the functions or update the comment.
🤖 Prompt for AI Agents
In Kernel/Process/Process.c around lines 54 to 67 and 113 to 114, the comment
states that ShouldSchedule and RequestSchedule were removed, but these functions
are still defined. To fix this inconsistency, either remove the definitions of
ShouldSchedule and RequestSchedule if they are truly unused, or update the
comment at lines 113-114 to accurately reflect that these functions remain in
the code.
| int ShouldSchedule(void) { | ||
| if (need_schedule) { | ||
| need_schedule = 0; | ||
| need_schedule = 0; // Atomically reset the flag | ||
| return 1; | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Sets a flag indicating the scheduler should run at the next opportunity. | ||
| */ | ||
| void RequestSchedule(void) { | ||
| need_schedule = 1; | ||
| } | ||
|
|
||
| void ProcessInit(void) { | ||
| // Clear all processes array | ||
| FastMemset(processes, 0, sizeof(Process) * MAX_PROCESSES); | ||
| void Yield() { | ||
| RequestSchedule(); | ||
| __asm__ __volatile__("hlt"); | ||
| } |
There was a problem hiding this comment.
Scheduling request mechanism not integrated with scheduler.
The need_schedule flag is set by RequestSchedule() and checked by ShouldSchedule(), but the actual scheduler (ScheduleFromInterrupt) never checks this flag. This means Yield() won't trigger rescheduling as intended.
Consider either:
- Removing these functions if not needed
- Integrating the
need_schedulecheck into the scheduler - Using a different mechanism to trigger scheduling on yield
🤖 Prompt for AI Agents
In Kernel/Process/Process.c around lines 54 to 72, the need_schedule flag set by
RequestSchedule() and checked by ShouldSchedule() is not integrated with the
actual scheduler, so Yield() does not trigger rescheduling. To fix this, either
remove the RequestSchedule, ShouldSchedule, and need_schedule flag if unused, or
modify the scheduler function (e.g., ScheduleFromInterrupt) to check the
need_schedule flag and perform scheduling accordingly. Alternatively, implement
a different mechanism to ensure Yield() triggers the scheduler as intended.
| // Find a free process slot. | ||
| int slot = -1; | ||
| for (int i = 1; i < MAX_PROCESSES; i++) { | ||
| for (int i = 0; i < MAX_PROCESSES; i++) { // Check all slots | ||
| if (processes[i].state == PROC_TERMINATED) { | ||
| slot = i; | ||
| break; | ||
| } | ||
| } | ||
| // If no terminated slots, use the next available one. | ||
| if (slot == -1) { | ||
| Panic("CreateSecureProcess: No free process slots"); | ||
| if (next_pid <= MAX_PROCESSES) { | ||
| slot = process_count; // Use the next empty slot | ||
| } else { | ||
| Panic("CreateSecureProcess: No free process slots"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Process slot allocation logic has potential bugs.
The current implementation has issues:
- Using
process_countas slot index (line 186) assumes sequential allocation - Condition
next_pid <= MAX_PROCESSES(line 185) incorrectly mixes PID count with slot count
- // Find a free process slot.
- int slot = -1;
- for (int i = 0; i < MAX_PROCESSES; i++) { // Check all slots
- if (processes[i].state == PROC_TERMINATED) {
- slot = i;
- break;
- }
- }
- // If no terminated slots, use the next available one.
- if (slot == -1) {
- if (next_pid <= MAX_PROCESSES) {
- slot = process_count; // Use the next empty slot
- } else {
- Panic("CreateSecureProcess: No free process slots");
- }
- }
+ // Find a free process slot.
+ int slot = -1;
+ for (int i = 0; i < MAX_PROCESSES; i++) {
+ if (processes[i].state == PROC_TERMINATED || processes[i].pid == 0) {
+ slot = i;
+ break;
+ }
+ }
+
+ if (slot == -1) {
+ Panic("CreateSecureProcess: No free process slots");
+ }🤖 Prompt for AI Agents
In Kernel/Process/Process.c around lines 175 to 190, the process slot allocation
logic is flawed because it incorrectly uses process_count as the slot index and
mixes next_pid with slot count in the condition. To fix this, replace the
condition to check for available slots properly without conflating PID and slot
counts, and ensure the slot index is assigned based on actual free slots or a
valid next available index rather than process_count. Adjust the logic to
reliably find or assign a free process slot without assumptions of sequential
allocation.
| for (uint32_t i = 1; i <= process_count; i++) { | ||
| uint32_t candidate_idx = (current_process + i) % MAX_PROCESSES; | ||
|
|
||
| if ((processes[candidate_idx].state == PROC_READY || processes[candidate_idx].state == PROC_RUNNING) | ||
| && processes[candidate_idx].pid != 0) // Don't schedule the idle process unless nothing else is ready | ||
| { | ||
| if (processes[candidate_idx].priority < best_priority) { | ||
| best_priority = processes[candidate_idx].priority; | ||
| next_proc_idx = candidate_idx; | ||
| } | ||
| } | ||
| next = (next + 1) % MAX_PROCESSES; | ||
| } while (next != start); | ||
|
|
||
| // Switch to best candidate if different from current | ||
| if (best_candidate != current_process) { | ||
| // Save current process's full interrupt frame | ||
| // Now this is safe because ProcessContext == struct Registers | ||
| FastMemcpy(&processes[current_process].context, regs, sizeof(struct Registers)); | ||
|
|
||
| // Update current process state | ||
| if (processes[current_process].state == PROC_RUNNING) { | ||
| processes[current_process].state = PROC_READY; | ||
| } | ||
|
|
||
| // Update new process state | ||
| processes[best_candidate].state = PROC_RUNNING; | ||
| } |
There was a problem hiding this comment.
Scheduler loop bounds may miss processes.
The loop uses i <= process_count but iterates over MAX_PROCESSES slots with modulo. If processes aren't contiguously allocated (e.g., after some terminate), this could miss valid processes.
- // Round-robin search for the highest priority (lowest value) process
- for (uint32_t i = 1; i <= process_count; i++) {
- uint32_t candidate_idx = (current_process + i) % MAX_PROCESSES;
+ // Round-robin search for the highest priority (lowest value) process
+ for (uint32_t i = 1; i < MAX_PROCESSES; i++) {
+ uint32_t candidate_idx = (current_process + i) % MAX_PROCESSES;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (uint32_t i = 1; i <= process_count; i++) { | |
| uint32_t candidate_idx = (current_process + i) % MAX_PROCESSES; | |
| if ((processes[candidate_idx].state == PROC_READY || processes[candidate_idx].state == PROC_RUNNING) | |
| && processes[candidate_idx].pid != 0) // Don't schedule the idle process unless nothing else is ready | |
| { | |
| if (processes[candidate_idx].priority < best_priority) { | |
| best_priority = processes[candidate_idx].priority; | |
| next_proc_idx = candidate_idx; | |
| } | |
| } | |
| next = (next + 1) % MAX_PROCESSES; | |
| } while (next != start); | |
| // Switch to best candidate if different from current | |
| if (best_candidate != current_process) { | |
| // Save current process's full interrupt frame | |
| // Now this is safe because ProcessContext == struct Registers | |
| FastMemcpy(&processes[current_process].context, regs, sizeof(struct Registers)); | |
| // Update current process state | |
| if (processes[current_process].state == PROC_RUNNING) { | |
| processes[current_process].state = PROC_READY; | |
| } | |
| // Update new process state | |
| processes[best_candidate].state = PROC_RUNNING; | |
| } | |
| // Round-robin search for the highest priority (lowest value) process | |
| for (uint32_t i = 1; i < MAX_PROCESSES; i++) { | |
| uint32_t candidate_idx = (current_process + i) % MAX_PROCESSES; | |
| if ((processes[candidate_idx].state == PROC_READY || processes[candidate_idx].state == PROC_RUNNING) | |
| && processes[candidate_idx].pid != 0) // Don't schedule the idle process unless nothing else is ready | |
| { | |
| if (processes[candidate_idx].priority < best_priority) { | |
| best_priority = processes[candidate_idx].priority; | |
| next_proc_idx = candidate_idx; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In Kernel/Process/Process.c around lines 247 to 258, the scheduler loop uses i
<= process_count which may skip valid processes if they are not contiguous.
Change the loop to iterate exactly MAX_PROCESSES times to cover all possible
process slots, ensuring no valid process is missed regardless of allocation
gaps.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores