From 8bd46306966551644cd0ef7f67034adda737a2c8 Mon Sep 17 00:00:00 2001 From: Klim Tsoutsman Date: Fri, 23 Dec 2022 23:06:14 +1100 Subject: [PATCH 01/12] Add task cancellation test It doesn't currently pass, but should in the near future. Signed-off-by: Klim Tsoutsman --- Cargo.lock | 11 ++++++ applications/test_task_cancel/Cargo.toml | 12 ++++++ applications/test_task_cancel/src/lib.rs | 49 ++++++++++++++++++++++++ theseus_features/Cargo.toml | 2 + 4 files changed, 74 insertions(+) create mode 100644 applications/test_task_cancel/Cargo.toml create mode 100644 applications/test_task_cancel/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 355bcbc24f..80f61a1001 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3781,6 +3781,16 @@ dependencies = [ "theseus_std", ] +[[package]] +name = "test_task_cancel" +version = "0.1.0" +dependencies = [ + "log", + "spawn", + "spin 0.9.4", + "task", +] + [[package]] name = "test_thread_local" version = "0.1.0" @@ -3894,6 +3904,7 @@ dependencies = [ "test_restartable", "test_serial_echo", "test_std_fs", + "test_task_cancel", "test_thread_local", "test_wait_queue", "test_wasmtime", diff --git a/applications/test_task_cancel/Cargo.toml b/applications/test_task_cancel/Cargo.toml new file mode 100644 index 0000000000..604b86d564 --- /dev/null +++ b/applications/test_task_cancel/Cargo.toml @@ -0,0 +1,12 @@ +[package] +name = "test_task_cancel" +version = "0.1.0" +authors = ["Klim Tsoutsman "] +description = "Task cancellation test" +edition = "2021" + +[dependencies] +log = "0.4" +spawn = { path = "../../kernel/spawn" } +spin = "0.9" +task = { path = "../../kernel/task" } diff --git a/applications/test_task_cancel/src/lib.rs b/applications/test_task_cancel/src/lib.rs new file mode 100644 index 0000000000..5f135d3d25 --- /dev/null +++ b/applications/test_task_cancel/src/lib.rs @@ -0,0 +1,49 @@ +// TODO: Properly implement Task::kill so the test passes. + +#![no_std] + +extern crate alloc; + +use alloc::{string::String, sync::Arc, vec::Vec}; +use core::sync::atomic::{AtomicUsize, Ordering}; +use spin::Mutex; + +pub fn main(_: Vec) -> isize { + let lock = Arc::new(Mutex::new(())); + let task = spawn::new_task_builder(other, lock.clone()) + .spawn() + .expect("failed to spawn task"); + + while !lock.is_locked() {} + + task.kill(task::KillReason::Requested) + .expect("failed to abort task"); + + log::debug!("waiting for lock to be unlocked"); + + // For us to acquire the lock, the drop handler of the other thread's guard must + // have been invoked. + let _ = lock.lock(); + + 0 +} + +#[inline(never)] +fn other(lock: Arc>) { + let _guard = lock.lock(); + loop { + // In order to properly unwind the task we need to reach an instruction that is + // covered by the unwind tables. Rust generates an unwind row for all call + // sites so by placing a call site in the loop we ensure that the task will + // reach an instruction from which it can unwind when it is told to cancel. + unwind_row_generator(); + } +} + +#[inline(never)] +fn unwind_row_generator() { + static __COUNTER: AtomicUsize = AtomicUsize::new(0); + + // Prevents unwind_row_generator from being optimised away. + __COUNTER.fetch_add(1, Ordering::Relaxed); +} diff --git a/theseus_features/Cargo.toml b/theseus_features/Cargo.toml index 56418ed7b5..cd648b05b2 100644 --- a/theseus_features/Cargo.toml +++ b/theseus_features/Cargo.toml @@ -63,6 +63,7 @@ test_realtime = { path = "../applications/test_realtime", optional = true } test_restartable = { path = "../applications/test_restartable", optional = true } test_serial_echo = { path = "../applications/test_serial_echo", optional = true } test_std_fs = { path = "../applications/test_std_fs", optional = true } +test_task_cancel = { path = "../applications/test_task_cancel", optional = true } test_wait_queue = { path = "../applications/test_wait_queue", optional = true } test_wasmtime = { path = "../applications/test_wasmtime", optional = true } tls_test = { path = "../applications/tls_test", optional = true } @@ -147,6 +148,7 @@ theseus_tests = [ "test_restartable", "test_serial_echo", "test_std_fs", + "test_task_cancel", "test_wait_queue", "test_wasmtime", "tls_test", From a6e4596827ae2358f5341179b3d6ef4c40a8b9b6 Mon Sep 17 00:00:00 2001 From: Klim Tsoutsman Date: Sun, 25 Dec 2022 23:46:13 +1100 Subject: [PATCH 02/12] Modify `test_task_cancel` to generate LSDA for `guard_holder` Signed-off-by: Klim Tsoutsman --- applications/test_task_cancel/src/lib.rs | 27 +++++++++++++++--------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/applications/test_task_cancel/src/lib.rs b/applications/test_task_cancel/src/lib.rs index 5f135d3d25..7f71aa8460 100644 --- a/applications/test_task_cancel/src/lib.rs +++ b/applications/test_task_cancel/src/lib.rs @@ -5,12 +5,12 @@ extern crate alloc; use alloc::{string::String, sync::Arc, vec::Vec}; -use core::sync::atomic::{AtomicUsize, Ordering}; +use core::sync::atomic::{AtomicBool, AtomicUsize, Ordering::Relaxed}; use spin::Mutex; pub fn main(_: Vec) -> isize { let lock = Arc::new(Mutex::new(())); - let task = spawn::new_task_builder(other, lock.clone()) + let task = spawn::new_task_builder(guard_hog, lock.clone()) .spawn() .expect("failed to spawn task"); @@ -29,21 +29,28 @@ pub fn main(_: Vec) -> isize { } #[inline(never)] -fn other(lock: Arc>) { +fn guard_hog(lock: Arc>) { let _guard = lock.lock(); loop { - // In order to properly unwind the task we need to reach an instruction that is - // covered by the unwind tables. Rust generates an unwind row for all call - // sites so by placing a call site in the loop we ensure that the task will - // reach an instruction from which it can unwind when it is told to cancel. - unwind_row_generator(); + // We cannot inline the load of FALSE into this function as then LLVM will + // generate an unwind row that covers the entire function, but a call site table + // that only covers the instructions associated with the panic, which we would + // never reach. + lsda_generator(); } } #[inline(never)] -fn unwind_row_generator() { +fn lsda_generator() { + static FALSE: AtomicBool = AtomicBool::new(false); static __COUNTER: AtomicUsize = AtomicUsize::new(0); + // We need to give LLVM false hope that unwind_row_generator may unwind. + // Otherwise, LLVM will generate an unwind row, but no LSDA for guard_holder. + if FALSE.load(Relaxed) { + panic!(); + } + // Prevents unwind_row_generator from being optimised away. - __COUNTER.fetch_add(1, Ordering::Relaxed); + __COUNTER.fetch_add(1, Relaxed); } From 66014431592dc2257b2d785aec4015fb93430572 Mon Sep 17 00:00:00 2001 From: Klim Tsoutsman Date: Mon, 26 Dec 2022 00:12:27 +1100 Subject: [PATCH 03/12] Working Signed-off-by: Klim Tsoutsman --- Cargo.lock | 18 ++++- applications/test_task_cancel/Cargo.toml | 2 +- applications/test_task_cancel/src/lib.rs | 3 +- kernel/exceptions_full/Cargo.toml | 3 + kernel/exceptions_full/src/lib.rs | 8 +- kernel/interrupts/Cargo.toml | 3 + kernel/interrupts/src/lib.rs | 8 +- kernel/kernel_config/src/memory.rs | 2 +- kernel/task/src/lib.rs | 12 ++- kernel/task_cancel/Cargo.toml | 16 ++++ kernel/task_cancel/src/lib.rs | 96 ++++++++++++++++++++++++ kernel/unwind/src/lib.rs | 25 ++++-- 12 files changed, 177 insertions(+), 19 deletions(-) create mode 100644 kernel/task_cancel/Cargo.toml create mode 100644 kernel/task_cancel/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 80f61a1001..689548d273 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1034,6 +1034,7 @@ dependencies = [ "signal_handler", "stack_trace", "task", + "task_cancel", "tlb_shootdown", "tss", "unwind", @@ -1487,6 +1488,7 @@ dependencies = [ "scheduler", "sleep", "spin 0.9.4", + "task", "tss", "vga_buffer", "x86_64", @@ -3589,6 +3591,20 @@ dependencies = [ "x86_64", ] +[[package]] +name = "task_cancel" +version = "0.1.0" +dependencies = [ + "gimli", + "log", + "memory_structs", + "mod_mgmt", + "spawn", + "task", + "unwind", + "x86_64", +] + [[package]] name = "task_fs" version = "0.1.0" @@ -3788,7 +3804,7 @@ dependencies = [ "log", "spawn", "spin 0.9.4", - "task", + "task_cancel", ] [[package]] diff --git a/applications/test_task_cancel/Cargo.toml b/applications/test_task_cancel/Cargo.toml index 604b86d564..6ffcc19faa 100644 --- a/applications/test_task_cancel/Cargo.toml +++ b/applications/test_task_cancel/Cargo.toml @@ -9,4 +9,4 @@ edition = "2021" log = "0.4" spawn = { path = "../../kernel/spawn" } spin = "0.9" -task = { path = "../../kernel/task" } +task_cancel = { path = "../../kernel/task_cancel" } diff --git a/applications/test_task_cancel/src/lib.rs b/applications/test_task_cancel/src/lib.rs index 7f71aa8460..d6ca63b3e3 100644 --- a/applications/test_task_cancel/src/lib.rs +++ b/applications/test_task_cancel/src/lib.rs @@ -16,8 +16,7 @@ pub fn main(_: Vec) -> isize { while !lock.is_locked() {} - task.kill(task::KillReason::Requested) - .expect("failed to abort task"); + task_cancel::cancel_task(task.clone()); log::debug!("waiting for lock to be unlocked"); diff --git a/kernel/exceptions_full/Cargo.toml b/kernel/exceptions_full/Cargo.toml index 671944a635..7323b9c26f 100644 --- a/kernel/exceptions_full/Cargo.toml +++ b/kernel/exceptions_full/Cargo.toml @@ -52,5 +52,8 @@ path = "../debug_info" [dependencies.signal_handler] path = "../signal_handler" +[dependencies.task_cancel] +path = "../task_cancel" + [lib] crate-type = ["rlib"] diff --git a/kernel/exceptions_full/src/lib.rs b/kernel/exceptions_full/src/lib.rs index e032967d13..5d7379b5b2 100644 --- a/kernel/exceptions_full/src/lib.rs +++ b/kernel/exceptions_full/src/lib.rs @@ -29,7 +29,7 @@ pub fn init(idt_ref: &'static LockedIdt) { // SET UP FIXED EXCEPTION HANDLERS idt.divide_error.set_handler_fn(divide_error_handler); - idt.debug.set_handler_fn(debug_handler); + idt.debug.set_handler_fn(task_cancel::interrupt_handler); idt.non_maskable_interrupt.set_handler_fn(nmi_handler); idt.breakpoint.set_handler_fn(breakpoint_handler); idt.overflow.set_handler_fn(overflow_handler); @@ -261,12 +261,6 @@ extern "x86-interrupt" fn divide_error_handler(stack_frame: InterruptStackFrame) kill_and_halt(0x0, &stack_frame, None, true) } -/// exception 0x01 -extern "x86-interrupt" fn debug_handler(stack_frame: InterruptStackFrame) { - println_both!("\nEXCEPTION: DEBUG EXCEPTION\n{:#X?}", stack_frame); - // don't halt here, this isn't a fatal/permanent failure, just a brief pause. -} - /// exception 0x02, also used for TLB Shootdown IPIs and sampling interrupts. /// /// # Important Note diff --git a/kernel/interrupts/Cargo.toml b/kernel/interrupts/Cargo.toml index c4373d5bbe..f817a4b5ce 100644 --- a/kernel/interrupts/Cargo.toml +++ b/kernel/interrupts/Cargo.toml @@ -38,5 +38,8 @@ path = "../sleep" [dependencies.vga_buffer] path = "../vga_buffer" +[dependencies.task] +path = "../task" + [lib] crate-type = ["rlib"] diff --git a/kernel/interrupts/src/lib.rs b/kernel/interrupts/src/lib.rs index d031fdcea8..5a6b475374 100644 --- a/kernel/interrupts/src/lib.rs +++ b/kernel/interrupts/src/lib.rs @@ -297,7 +297,7 @@ pub fn eoi(irq: Option) { pub static APIC_TIMER_TICKS: AtomicUsize = AtomicUsize::new(0); /// 0x22 -extern "x86-interrupt" fn lapic_timer_handler(_stack_frame: InterruptStackFrame) { +extern "x86-interrupt" fn lapic_timer_handler(mut stack_frame: InterruptStackFrame) { let _ticks = APIC_TIMER_TICKS.fetch_add(1, Ordering::Relaxed); // info!(" ({}) APIC TIMER HANDLER! TICKS = {}", apic::current_cpu(), _ticks); @@ -310,6 +310,12 @@ extern "x86-interrupt" fn lapic_timer_handler(_stack_frame: InterruptStackFrame) eoi(None); // None, because 0x22 IRQ cannot possibly be a PIC interrupt scheduler::schedule(); + + if let Some(current_task) = task::get_my_current_task() { + if current_task.is_cancelled() { + unsafe { stack_frame.as_mut() }.update(|stack_frame| stack_frame.cpu_flags |= 0x100); + } + } } extern "x86-interrupt" fn apic_spurious_interrupt_handler(_stack_frame: InterruptStackFrame) { diff --git a/kernel/kernel_config/src/memory.rs b/kernel/kernel_config/src/memory.rs index d2b4d02e49..1cd4348b34 100644 --- a/kernel/kernel_config/src/memory.rs +++ b/kernel/kernel_config/src/memory.rs @@ -55,7 +55,7 @@ pub const MAX_PAGE_NUMBER: usize = MAX_VIRTUAL_ADDRESS / PAGE_SIZE; /// The size in pages of each kernel stack. /// If it's too small, complex kernel functions will overflow, causing a page fault / double fault. #[cfg(not(debug_assertions))] -pub const KERNEL_STACK_SIZE_IN_PAGES: usize = 16; +pub const KERNEL_STACK_SIZE_IN_PAGES: usize = 32; #[cfg(debug_assertions)] pub const KERNEL_STACK_SIZE_IN_PAGES: usize = 32; // debug builds require more stack space. diff --git a/kernel/task/src/lib.rs b/kernel/task/src/lib.rs index 9dcfa251db..230bc78ca0 100755 --- a/kernel/task/src/lib.rs +++ b/kernel/task/src/lib.rs @@ -365,6 +365,7 @@ pub struct Task { /// /// This is not public because it permits interior mutability. joinable: AtomicBool, + pub cancel_requested: AtomicBool, /// Memory management details: page tables, mappings, allocators, etc. /// This is shared among all other tasks in the same address space. pub mmi: MmiRef, @@ -514,6 +515,7 @@ impl Task { suspended: AtomicBool::new(false), // Tasks are not considered "joinable" until passed to `TaskRef::new()` joinable: AtomicBool::new(false), + cancel_requested: AtomicBool::new(false), mmi, is_an_idle_task: false, app_crate, @@ -796,6 +798,15 @@ impl Task { pub fn is_suspended(&self) -> bool { self.suspended.load(Ordering::Acquire) } + + /// Returns `true` if this task is cancelled. + /// + /// # Latency + /// + /// The cancellation of a task will take some time to propagate to other cores. + pub fn is_cancelled(&self) -> bool { + self.cancel_requested.load(Ordering::Relaxed) + } /// Sets the waker to be awoken when this task exits. pub fn set_waker(&self, waker: Waker) { @@ -892,7 +903,6 @@ pub fn task_switch( apic_id: u8, preemption_guard: PreemptionGuard, ) -> (bool, PreemptionGuard) { - // We use the `with_current_task_and_value()` closure here in order to ensure that // the borrowed reference to the current task is guaranteed to be dropped // *before* the actual context switch operation occurs. diff --git a/kernel/task_cancel/Cargo.toml b/kernel/task_cancel/Cargo.toml new file mode 100644 index 0000000000..d3a31cd157 --- /dev/null +++ b/kernel/task_cancel/Cargo.toml @@ -0,0 +1,16 @@ +[package] +name = "task_cancel" +version = "0.1.0" +authors = ["Klim Tsoutsman "] +description = "Task cancellation" +edition = "2021" + +[dependencies] +gimli = { version = "0.25", default-features = false } +memory_structs = { path = "../memory_structs" } +mod_mgmt = { path = "../mod_mgmt" } +spawn = { path = "../spawn" } +task = { path = "../task" } +unwind = { path = "../unwind" } +x86_64 = "0.14" +log = "*" diff --git a/kernel/task_cancel/src/lib.rs b/kernel/task_cancel/src/lib.rs new file mode 100644 index 0000000000..02ed3eeaa7 --- /dev/null +++ b/kernel/task_cancel/src/lib.rs @@ -0,0 +1,96 @@ +// TODO: Move into task crate. Hasn't yet been done because it creates a +// circular dependency as unwind depends on task. + +#![feature(abi_x86_interrupt, try_blocks)] +#![no_std] + +use core::{default::Default, iter::Iterator, sync::atomic::Ordering}; +use gimli::{BaseAddresses, EhFrame, NativeEndian, UninitializedUnwindContext, UnwindSection}; +use memory_structs::VirtualAddress; +use mod_mgmt::SectionType; +use task::{KillReason, TaskRef}; +use x86_64::structures::idt::InterruptStackFrame; + +pub fn cancel_task(task: TaskRef) { + task.cancel_requested.store(true, Ordering::Relaxed); +} + +pub fn set_trap_flag(stack_frame: &mut InterruptStackFrame) { + unsafe { stack_frame.as_mut() }.update(|stack_frame| stack_frame.cpu_flags |= 0x100); +} + +pub extern "x86-interrupt" fn interrupt_handler(mut stack_frame: InterruptStackFrame) { + let instruction_pointer = stack_frame.instruction_pointer.as_u64(); + let stack_pointer = stack_frame.stack_pointer.as_u64(); + + log::info!("instruction pointer: {instruction_pointer:0x?}"); + + let can_unwind = Option::<_>::is_some( + &try { + let task = task::get_my_current_task().expect("couldn't get current task"); + + // TODO: Search external unwind info. + + let krate = task.namespace.get_crate_containing_address( + VirtualAddress::new_canonical(instruction_pointer as usize), + false, + )?; + + let krate = krate.lock_as_ref(); + let text_address = krate.text_pages.as_ref()?.1.start.value(); + let eh_frame_section = krate + .sections + .values() + .find(|s| s.typ == SectionType::EhFrame)?; + let eh_frame_address = eh_frame_section.virt_addr.value(); + + let base_addresses = BaseAddresses::default() + .set_text(text_address as u64) + .set_eh_frame(eh_frame_address as u64); + log::info!("{base_addresses:0x?}"); + + let pages = eh_frame_section.mapped_pages.lock(); + let bytes = pages + .as_slice(eh_frame_section.mapped_pages_offset, eh_frame_section.size) + .ok()?; + let eh_frame = EhFrame::new(bytes, NativeEndian); + + let frame_description_entry = eh_frame + .fde_for_address( + &base_addresses, + instruction_pointer, + EhFrame::cie_from_offset, + ) + .ok()?; + + let mut unwind_context = UninitializedUnwindContext::new(); + let _ = frame_description_entry + .unwind_info_for_address( + &eh_frame, + &base_addresses, + &mut unwind_context, + instruction_pointer, + ) + .ok()?; + + log::info!("covering"); + + Some(()) + }, + ); + + log::info!("stack frame: {:0x?}", stack_frame.stack_pointer); + + if can_unwind { + unwind::start_remote_unwinding( + KillReason::Requested, + 0, + stack_pointer, + instruction_pointer, + ) + .expect("failed to unwind"); + } else { + // FIXME: What happens if the APIC interrupt triggers here? + set_trap_flag(&mut stack_frame); + } +} diff --git a/kernel/unwind/src/lib.rs b/kernel/unwind/src/lib.rs index 87a8870132..530d793ca2 100644 --- a/kernel/unwind/src/lib.rs +++ b/kernel/unwind/src/lib.rs @@ -718,7 +718,6 @@ fn get_eh_frame_info(crate_ref: &StrongCrateRef) -> Option<(StrongSectionRef, Ba Some((eh_frame_sec.clone(), base_addrs)) } - /// Starts the unwinding procedure for the current task /// by working backwards up the call stack starting from the current stack frame. /// @@ -731,9 +730,19 @@ fn get_eh_frame_info(crate_ref: &StrongCrateRef) -> Option<(StrongSectionRef, Ba /// ## Note: Skipping frames /// If you are unsure how many frames you could possibly skip, then it's always safe to pass `0` /// such that all function frames on the stack are unwound. -/// -#[doc(hidden)] pub fn start_unwinding(reason: KillReason, stack_frames_to_skip: usize) -> Result<(), &'static str> { + start_unwinding_inner(reason, stack_frames_to_skip, None, None) +} + +/// Starts the unwinding procedure for a specific instruction pointer, and stack pointer. +/// +/// See [`start_unwinding`] for more information. +// TODO: Rename. This isn't technically remote unwinding. +pub fn start_remote_unwinding(reason: KillReason, stack_frames_to_skip: usize, rsp: u64, ip: u64) -> Result<(), &'static str> { + start_unwinding_inner(reason, stack_frames_to_skip, Some(rsp), Some(ip)) +} + +fn start_unwinding_inner(reason: KillReason, stack_frames_to_skip: usize, rsp: Option, ip: Option) -> Result<(), &'static str> { // Here we have to be careful to have no resources waiting to be dropped/freed/released on the stack. let unwinding_context_ptr = { let current_task = task::get_my_current_task().ok_or("couldn't get current task")?; @@ -757,11 +766,18 @@ pub fn start_unwinding(reason: KillReason, stack_frames_to_skip: usize) -> Resul // We pass a pointer to the unwinding context to this closure. - let res = invoke_with_current_registers(&mut |registers| { + let res = invoke_with_current_registers(&mut |mut registers: Registers| { // set the proper register values before start the actual unwinding procedure. { // SAFE: we just created this pointer above let unwinding_context = unsafe { &mut *unwinding_context_ptr }; + if let Some(rsp) = rsp { + registers[X86_64::RSP] = Some(rsp); + } + if let Some(ip) = ip { + // Accounts for the -1 later + registers[X86_64::RA] = Some(ip + 1); + } unwinding_context.stack_frame_iter.registers = registers; // Skip the first several frames, e.g., to skip unwinding the panic entry point functions themselves. @@ -789,7 +805,6 @@ pub fn start_unwinding(reason: KillReason, stack_frames_to_skip: usize) -> Resul cleanup_unwinding_context(unwinding_context_ptr); } - /// Continues the unwinding process from the point it left off at, /// which is defined by the given unwinding context. /// From 4edd99fd13d04431247b873053c773a6970a8edd Mon Sep 17 00:00:00 2001 From: Klim Tsoutsman Date: Sun, 25 Dec 2022 23:46:13 +1100 Subject: [PATCH 04/12] Modify `test_task_cancel` to generate LSDA for `guard_holder` Signed-off-by: Klim Tsoutsman --- applications/test_task_cancel/src/lib.rs | 31 ++++++++++++++---------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/applications/test_task_cancel/src/lib.rs b/applications/test_task_cancel/src/lib.rs index 5f135d3d25..64b6fce4ce 100644 --- a/applications/test_task_cancel/src/lib.rs +++ b/applications/test_task_cancel/src/lib.rs @@ -5,12 +5,12 @@ extern crate alloc; use alloc::{string::String, sync::Arc, vec::Vec}; -use core::sync::atomic::{AtomicUsize, Ordering}; +use core::sync::atomic::{AtomicBool, Ordering::Relaxed}; use spin::Mutex; pub fn main(_: Vec) -> isize { let lock = Arc::new(Mutex::new(())); - let task = spawn::new_task_builder(other, lock.clone()) + let task = spawn::new_task_builder(guard_hog, lock.clone()) .spawn() .expect("failed to spawn task"); @@ -29,21 +29,26 @@ pub fn main(_: Vec) -> isize { } #[inline(never)] -fn other(lock: Arc>) { +fn guard_hog(lock: Arc>) { let _guard = lock.lock(); loop { - // In order to properly unwind the task we need to reach an instruction that is - // covered by the unwind tables. Rust generates an unwind row for all call - // sites so by placing a call site in the loop we ensure that the task will - // reach an instruction from which it can unwind when it is told to cancel. - unwind_row_generator(); + // We cannot inline the load of FALSE into this function as then LLVM will + // generate an unwind row that covers the entire function, but a call site table + // that only covers the instructions associated with the panic, which we would + // never reach. + lsda_generator(); } } #[inline(never)] -fn unwind_row_generator() { - static __COUNTER: AtomicUsize = AtomicUsize::new(0); - - // Prevents unwind_row_generator from being optimised away. - __COUNTER.fetch_add(1, Ordering::Relaxed); +fn lsda_generator() { + static FALSE: AtomicBool = AtomicBool::new(false); + + // We need to give LLVM false hope that lsda_generator may unwind. Otherwise, + // LLVM will generate an unwind row, but no LSDA for guard_holder. + // + // The potential panic also prevents lsda_generator from being optimised away. + if FALSE.load(Relaxed) { + panic!(); + } } From cd80e0af341260b428c61d678af7d714792fca91 Mon Sep 17 00:00:00 2001 From: Klim Tsoutsman Date: Mon, 26 Dec 2022 14:04:21 +1100 Subject: [PATCH 05/12] Move `can_unwind` into `unwind` Signed-off-by: Klim Tsoutsman --- kernel/task_cancel/src/lib.rs | 70 +++--------------------------- kernel/unwind/src/lib.rs | 82 ++++++++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 64 deletions(-) diff --git a/kernel/task_cancel/src/lib.rs b/kernel/task_cancel/src/lib.rs index 02ed3eeaa7..112007ffae 100644 --- a/kernel/task_cancel/src/lib.rs +++ b/kernel/task_cancel/src/lib.rs @@ -4,10 +4,7 @@ #![feature(abi_x86_interrupt, try_blocks)] #![no_std] -use core::{default::Default, iter::Iterator, sync::atomic::Ordering}; -use gimli::{BaseAddresses, EhFrame, NativeEndian, UninitializedUnwindContext, UnwindSection}; -use memory_structs::VirtualAddress; -use mod_mgmt::SectionType; +use core::sync::atomic::Ordering; use task::{KillReason, TaskRef}; use x86_64::structures::idt::InterruptStackFrame; @@ -15,7 +12,7 @@ pub fn cancel_task(task: TaskRef) { task.cancel_requested.store(true, Ordering::Relaxed); } -pub fn set_trap_flag(stack_frame: &mut InterruptStackFrame) { +fn set_trap_flag(stack_frame: &mut InterruptStackFrame) { unsafe { stack_frame.as_mut() }.update(|stack_frame| stack_frame.cpu_flags |= 0x100); } @@ -25,63 +22,8 @@ pub extern "x86-interrupt" fn interrupt_handler(mut stack_frame: InterruptStackF log::info!("instruction pointer: {instruction_pointer:0x?}"); - let can_unwind = Option::<_>::is_some( - &try { - let task = task::get_my_current_task().expect("couldn't get current task"); - - // TODO: Search external unwind info. - - let krate = task.namespace.get_crate_containing_address( - VirtualAddress::new_canonical(instruction_pointer as usize), - false, - )?; - - let krate = krate.lock_as_ref(); - let text_address = krate.text_pages.as_ref()?.1.start.value(); - let eh_frame_section = krate - .sections - .values() - .find(|s| s.typ == SectionType::EhFrame)?; - let eh_frame_address = eh_frame_section.virt_addr.value(); - - let base_addresses = BaseAddresses::default() - .set_text(text_address as u64) - .set_eh_frame(eh_frame_address as u64); - log::info!("{base_addresses:0x?}"); - - let pages = eh_frame_section.mapped_pages.lock(); - let bytes = pages - .as_slice(eh_frame_section.mapped_pages_offset, eh_frame_section.size) - .ok()?; - let eh_frame = EhFrame::new(bytes, NativeEndian); - - let frame_description_entry = eh_frame - .fde_for_address( - &base_addresses, - instruction_pointer, - EhFrame::cie_from_offset, - ) - .ok()?; - - let mut unwind_context = UninitializedUnwindContext::new(); - let _ = frame_description_entry - .unwind_info_for_address( - &eh_frame, - &base_addresses, - &mut unwind_context, - instruction_pointer, - ) - .ok()?; - - log::info!("covering"); - - Some(()) - }, - ); - - log::info!("stack frame: {:0x?}", stack_frame.stack_pointer); - - if can_unwind { + if unwind::can_unwind(instruction_pointer) { + log::info!("unwinding a cancelled task"); unwind::start_remote_unwinding( KillReason::Requested, 0, @@ -90,7 +32,9 @@ pub extern "x86-interrupt" fn interrupt_handler(mut stack_frame: InterruptStackF ) .expect("failed to unwind"); } else { - // FIXME: What happens if the APIC interrupt triggers here? + // The trap flag is reset after every debug interrupt. Since we can't unwind at + // this instruction, we reset the flag to check again at the next instruction. set_trap_flag(&mut stack_frame); + // FIXME: What happens if a LAPIC timer interrupt triggers here? } } diff --git a/kernel/unwind/src/lib.rs b/kernel/unwind/src/lib.rs index 530d793ca2..ed7027fb11 100644 --- a/kernel/unwind/src/lib.rs +++ b/kernel/unwind/src/lib.rs @@ -413,7 +413,7 @@ impl FallibleIterator for StackFrameIter { let size_of_exception_stack_frame: i64 = 5 * 8; #[cfg(not(downtime_eval))] trace!("StackFrameIter: next stack frame is an exception handler: adding {:#X} to cfa, new cfa: {:#X}", size_of_exception_stack_frame, cfa); - + this_frame_is_exception_handler = true; Some(size_of_error_code + size_of_exception_stack_frame) } else { @@ -456,6 +456,86 @@ unsafe fn deref_ptr(ptr: Pointer) -> u64 { pub trait FuncWithRegisters = FnMut(Registers) -> Result<(), &'static str>; type FuncWithRegistersRefMut<'a> = &'a mut dyn FuncWithRegisters; +/// Returns whether unwinding can begin from the given instruction pointer. +/// +/// This happens when one of the following two criteria is met: +/// +/// 1. The pointer is covered by the unwind tables, but does not have an LSDA. +/// This occurs when the function does not have any associated drop handlers, +/// either because it has nothing to drop or Rust has determined that the +/// function will never be unwound. +/// +/// 2. The pointer is covered by the unwind tables, has an LSDA, and is covered +/// by the LSDA's call site table. This occurs when the function has associated +/// drop handlers and the instruction pointer is at an instruction that could +/// cause an unwinding (usually a call site to a function that can cause an +/// unwinding). +pub fn can_unwind(instruction_pointer: u64) -> bool { + can_unwind_inner(instruction_pointer).is_some() +} + +fn can_unwind_inner(instruction_pointer: u64) -> Option<()> { + let task = task::get_my_current_task()?; + let instruction_address = VirtualAddress::new(instruction_pointer as usize)?; + let krate = task + .namespace + .get_crate_containing_address(instruction_address, false)?; + + let (eh_frame, base_addresses) = get_eh_frame_info(&krate) + .map(|(eh_frame_section, base_addresses)| { + (EhFrameReference::Section(eh_frame_section), base_addresses) + }) + .or_else(|| { + external_unwind_info::get_unwind_info(instruction_address).map(|unwind_info| { + let base_addresses = BaseAddresses::default() + .set_eh_frame(unwind_info.unwind_info.start.value() as u64) + .set_text(unwind_info.text_section.start.value() as u64); + (EhFrameReference::External(unwind_info), base_addresses) + }) + })?; + + let unwind_row = UnwindRowReference { + caller: instruction_pointer, + eh_frame_sec: eh_frame, + base_addrs: base_addresses, + }; + let StackFrame { + lsda: lsda_address, + initial_address, + call_site_address, + .. + } = unwind_row + .with_unwind_info(|fde, _| { + Ok(StackFrame { + personality: None, + lsda: fde.lsda().map(|address| unsafe { deref_ptr(address) }), + initial_address: fde.initial_address(), + call_site_address: instruction_pointer, + }) + }) + .ok()?; + + let lsda_address = match lsda_address { + Some(address) => VirtualAddress::new_canonical(address as usize), + None => return Some(()), + }; + + let (lsda_section, _) = task + .namespace + .get_section_containing_address(lsda_address, true)?; + let start = + lsda_section.mapped_pages_offset + (lsda_address.value() - lsda_section.virt_addr.value()); + let length = lsda_section.virt_addr.value() + lsda_section.size - lsda_address.value(); + + let lsda_pages = lsda_section.mapped_pages.lock(); + let lsda_slice = lsda_pages.as_slice::(start, length).ok()?; + let table = lsda::GccExceptTableArea::new(lsda_slice, NativeEndian, initial_address); + + table + .call_site_table_entry_for_address(call_site_address) + .ok() + .map(|_| ()) +} /// This function saves the current CPU register values onto the stack (to preserve them) /// and then invokes the given closure with those registers as the argument. From 074688b99e954afe9170edad844b4a1fab83c830 Mon Sep 17 00:00:00 2001 From: Klim Tsoutsman Date: Mon, 26 Dec 2022 14:16:34 +1100 Subject: [PATCH 06/12] Update docs Signed-off-by: Klim Tsoutsman --- kernel/unwind/src/lib.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/kernel/unwind/src/lib.rs b/kernel/unwind/src/lib.rs index ed7027fb11..4a96b76794 100644 --- a/kernel/unwind/src/lib.rs +++ b/kernel/unwind/src/lib.rs @@ -465,11 +465,10 @@ type FuncWithRegistersRefMut<'a> = &'a mut dyn FuncWithRegisters; /// either because it has nothing to drop or Rust has determined that the /// function will never be unwound. /// -/// 2. The pointer is covered by the unwind tables, has an LSDA, and is covered -/// by the LSDA's call site table. This occurs when the function has associated -/// drop handlers and the instruction pointer is at an instruction that could -/// cause an unwinding (usually a call site to a function that can cause an -/// unwinding). +/// 2. The pointer is covered by the unwind tables, and the LSDA's call site +/// table. This occurs when the function has associated drop handlers and the +/// instruction pointer is at an instruction that could cause an unwinding +/// (usually a call site to a function that can cause an unwinding). pub fn can_unwind(instruction_pointer: u64) -> bool { can_unwind_inner(instruction_pointer).is_some() } @@ -517,6 +516,7 @@ fn can_unwind_inner(instruction_pointer: u64) -> Option<()> { let lsda_address = match lsda_address { Some(address) => VirtualAddress::new_canonical(address as usize), + // Criteria 1: Covered by unwind tables, but no LSDA. None => return Some(()), }; @@ -531,6 +531,7 @@ fn can_unwind_inner(instruction_pointer: u64) -> Option<()> { let lsda_slice = lsda_pages.as_slice::(start, length).ok()?; let table = lsda::GccExceptTableArea::new(lsda_slice, NativeEndian, initial_address); + // Criteria 2: Covered by unwind tables, and LSDA's call site table. table .call_site_table_entry_for_address(call_site_address) .ok() From 3e5ffcef30256548550177a9081e88c4501340f0 Mon Sep 17 00:00:00 2001 From: Klim Tsoutsman Date: Mon, 26 Dec 2022 14:59:13 +1100 Subject: [PATCH 07/12] Change second unwind condition to require an associated landing pad Signed-off-by: Klim Tsoutsman --- applications/test_task_cancel/src/lib.rs | 5 ++++- kernel/kernel_config/src/memory.rs | 2 +- kernel/task_cancel/src/lib.rs | 3 +-- kernel/unwind/src/lib.rs | 19 +++++++++++-------- 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/applications/test_task_cancel/src/lib.rs b/applications/test_task_cancel/src/lib.rs index 299a76ebf7..252f7f3c18 100644 --- a/applications/test_task_cancel/src/lib.rs +++ b/applications/test_task_cancel/src/lib.rs @@ -1,4 +1,7 @@ -// TODO: Properly implement Task::kill so the test passes. +// TODO: Test that guard_hog is succesfully cancelled when at the call +// instruction, in which case it should unwind immediately, and at the jmp +// (loop) instruction, in which case it should continue to the next instruction, +// which is the call instruction, and then unwind. #![no_std] diff --git a/kernel/kernel_config/src/memory.rs b/kernel/kernel_config/src/memory.rs index 1cd4348b34..d2b4d02e49 100644 --- a/kernel/kernel_config/src/memory.rs +++ b/kernel/kernel_config/src/memory.rs @@ -55,7 +55,7 @@ pub const MAX_PAGE_NUMBER: usize = MAX_VIRTUAL_ADDRESS / PAGE_SIZE; /// The size in pages of each kernel stack. /// If it's too small, complex kernel functions will overflow, causing a page fault / double fault. #[cfg(not(debug_assertions))] -pub const KERNEL_STACK_SIZE_IN_PAGES: usize = 32; +pub const KERNEL_STACK_SIZE_IN_PAGES: usize = 16; #[cfg(debug_assertions)] pub const KERNEL_STACK_SIZE_IN_PAGES: usize = 32; // debug builds require more stack space. diff --git a/kernel/task_cancel/src/lib.rs b/kernel/task_cancel/src/lib.rs index 112007ffae..273ebc3d3a 100644 --- a/kernel/task_cancel/src/lib.rs +++ b/kernel/task_cancel/src/lib.rs @@ -20,8 +20,6 @@ pub extern "x86-interrupt" fn interrupt_handler(mut stack_frame: InterruptStackF let instruction_pointer = stack_frame.instruction_pointer.as_u64(); let stack_pointer = stack_frame.stack_pointer.as_u64(); - log::info!("instruction pointer: {instruction_pointer:0x?}"); - if unwind::can_unwind(instruction_pointer) { log::info!("unwinding a cancelled task"); unwind::start_remote_unwinding( @@ -32,6 +30,7 @@ pub extern "x86-interrupt" fn interrupt_handler(mut stack_frame: InterruptStackF ) .expect("failed to unwind"); } else { + log::debug!("couldn't unwind at {instruction_pointer:0x?}; resetting trap flag"); // The trap flag is reset after every debug interrupt. Since we can't unwind at // this instruction, we reset the flag to check again at the next instruction. set_trap_flag(&mut stack_frame); diff --git a/kernel/unwind/src/lib.rs b/kernel/unwind/src/lib.rs index 4a96b76794..0cbb0355df 100644 --- a/kernel/unwind/src/lib.rs +++ b/kernel/unwind/src/lib.rs @@ -465,9 +465,9 @@ type FuncWithRegistersRefMut<'a> = &'a mut dyn FuncWithRegisters; /// either because it has nothing to drop or Rust has determined that the /// function will never be unwound. /// -/// 2. The pointer is covered by the unwind tables, and the LSDA's call site -/// table. This occurs when the function has associated drop handlers and the -/// instruction pointer is at an instruction that could cause an unwinding +/// 2. The pointer is covered by the unwind tables, and has an associated +/// landing pad. This occurs when the function has associated drop handlers and +/// the instruction pointer is at an instruction that could cause an unwinding /// (usually a call site to a function that can cause an unwinding). pub fn can_unwind(instruction_pointer: u64) -> bool { can_unwind_inner(instruction_pointer).is_some() @@ -531,11 +531,12 @@ fn can_unwind_inner(instruction_pointer: u64) -> Option<()> { let lsda_slice = lsda_pages.as_slice::(start, length).ok()?; let table = lsda::GccExceptTableArea::new(lsda_slice, NativeEndian, initial_address); - // Criteria 2: Covered by unwind tables, and LSDA's call site table. - table + let entry = table .call_site_table_entry_for_address(call_site_address) - .ok() - .map(|_| ()) + .ok()?; + + // Criteria 2: Covered by unwind tables, and has an associated landing pad. + entry.landing_pad_address().map(|_| ()) } /// This function saves the current CPU register values onto the stack (to preserve them) @@ -856,7 +857,9 @@ fn start_unwinding_inner(reason: KillReason, stack_frames_to_skip: usize, rsp: O registers[X86_64::RSP] = Some(rsp); } if let Some(ip) = ip { - // Accounts for the -1 later + // StackFrameIter::next subtracts 1 from the return address to get the call site. + // Since we're already providing the call site instruction pointer, we add 1 here + // to negate the subtraction later. registers[X86_64::RA] = Some(ip + 1); } unwinding_context.stack_frame_iter.registers = registers; From 432cc1d60f09b70f8bdce71a893e608cb57a9ce2 Mon Sep 17 00:00:00 2001 From: Klim Tsoutsman Date: Mon, 26 Dec 2022 15:01:17 +1100 Subject: [PATCH 08/12] Remove unnecessary `task_cancel` dependencies Signed-off-by: Klim Tsoutsman --- Cargo.lock | 4 ---- kernel/task_cancel/Cargo.toml | 6 +----- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 689548d273..963dc21df9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3595,11 +3595,7 @@ dependencies = [ name = "task_cancel" version = "0.1.0" dependencies = [ - "gimli", "log", - "memory_structs", - "mod_mgmt", - "spawn", "task", "unwind", "x86_64", diff --git a/kernel/task_cancel/Cargo.toml b/kernel/task_cancel/Cargo.toml index d3a31cd157..ce33472fe3 100644 --- a/kernel/task_cancel/Cargo.toml +++ b/kernel/task_cancel/Cargo.toml @@ -6,11 +6,7 @@ description = "Task cancellation" edition = "2021" [dependencies] -gimli = { version = "0.25", default-features = false } -memory_structs = { path = "../memory_structs" } -mod_mgmt = { path = "../mod_mgmt" } -spawn = { path = "../spawn" } task = { path = "../task" } unwind = { path = "../unwind" } x86_64 = "0.14" -log = "*" +log = "0.4" From 5b8693018c1ac89bed40da86e6ed2aef3d8462e3 Mon Sep 17 00:00:00 2001 From: Klim Tsoutsman Date: Mon, 26 Dec 2022 15:04:29 +1100 Subject: [PATCH 09/12] Add documentation for trap flag in `interrupts` Signed-off-by: Klim Tsoutsman --- kernel/interrupts/src/lib.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/kernel/interrupts/src/lib.rs b/kernel/interrupts/src/lib.rs index 5a6b475374..95679541b9 100644 --- a/kernel/interrupts/src/lib.rs +++ b/kernel/interrupts/src/lib.rs @@ -313,11 +313,16 @@ extern "x86-interrupt" fn lapic_timer_handler(mut stack_frame: InterruptStackFra if let Some(current_task) = task::get_my_current_task() { if current_task.is_cancelled() { - unsafe { stack_frame.as_mut() }.update(|stack_frame| stack_frame.cpu_flags |= 0x100); + // Trigger a debug interrupt on the next instruction which will invoke task_cancel::interrupt_handler. + set_trap_flag(&mut stack_frame); } } } +fn set_trap_flag(stack_frame: &mut InterruptStackFrame) { + unsafe { stack_frame.as_mut() }.update(|stack_frame| stack_frame.cpu_flags |= 0x100); +} + extern "x86-interrupt" fn apic_spurious_interrupt_handler(_stack_frame: InterruptStackFrame) { warn!("APIC SPURIOUS INTERRUPT HANDLER!"); From 1a6e44e9bda8d37bdf61451f093a4fd8ad51d9ad Mon Sep 17 00:00:00 2001 From: Klim Tsoutsman Date: Mon, 26 Dec 2022 15:17:15 +1100 Subject: [PATCH 10/12] Better document `test_task_cancel` Signed-off-by: Klim Tsoutsman --- applications/test_task_cancel/src/lib.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/applications/test_task_cancel/src/lib.rs b/applications/test_task_cancel/src/lib.rs index 252f7f3c18..8cb339d794 100644 --- a/applications/test_task_cancel/src/lib.rs +++ b/applications/test_task_cancel/src/lib.rs @@ -1,14 +1,21 @@ -// TODO: Test that guard_hog is succesfully cancelled when at the call -// instruction, in which case it should unwind immediately, and at the jmp -// (loop) instruction, in which case it should continue to the next instruction, -// which is the call instruction, and then unwind. +// TODO: Test that the other thread is succesfully cancelled in the following +// scenarios: +// +// 1. In lsda_generator, in which case it should trigger the first criteria of +// unwind::can_unwind. +// +// 2. At the call lsda_generator instruction, in which case it should trigger +// the second criteria of unwind::can_unwind. +// +// 3. At the jmp (loop) instruction, in which case it should continue to the +// next (call) instruction and then unwind. #![no_std] extern crate alloc; use alloc::{string::String, sync::Arc, vec::Vec}; -use core::sync::atomic::{AtomicBool, Ordering::Relaxed}; +use core::sync::atomic::{AtomicBool, AtomicUsize, Ordering::Relaxed}; use spin::Mutex; pub fn main(_: Vec) -> isize { @@ -53,4 +60,8 @@ fn lsda_generator() { if FALSE.load(Relaxed) { panic!(); } + + // Spend more time in lsda_generator to increase likelihood of scenario 1. + static __COUNTER: AtomicUsize = AtomicUsize::new(0); + __COUNTER.fetch_add(1, Relaxed); } From adb3b0e0788e158e943beede01c30556e1758028 Mon Sep 17 00:00:00 2001 From: Klimenty Tsoutsman Date: Tue, 18 Jul 2023 18:29:03 +1000 Subject: [PATCH 11/12] Satisfy clippy Signed-off-by: Klimenty Tsoutsman --- kernel/scheduler/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/scheduler/src/lib.rs b/kernel/scheduler/src/lib.rs index 1dd14de132..32e712c7c7 100644 --- a/kernel/scheduler/src/lib.rs +++ b/kernel/scheduler/src/lib.rs @@ -92,7 +92,7 @@ interrupt_handler!(timer_tick_handler, None, mut stack_frame, { if let Some(current_task) = task::get_my_current_task() { if current_task.is_cancelled() { // Trigger a debug interrupt on the next instruction which will invoke task_cancel::interrupt_handler. - task_cancel::set_trap_flag(&mut stack_frame); + task_cancel::set_trap_flag(stack_frame); } } From bda973cd7b016694d14020270f06f50cc1219be7 Mon Sep 17 00:00:00 2001 From: Klimenty Tsoutsman Date: Sun, 10 Sep 2023 09:17:39 +1000 Subject: [PATCH 12/12] Clippy Signed-off-by: Klimenty Tsoutsman --- kernel/task_cancel/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/kernel/task_cancel/src/lib.rs b/kernel/task_cancel/src/lib.rs index 58a103544f..1b3b2faf56 100644 --- a/kernel/task_cancel/src/lib.rs +++ b/kernel/task_cancel/src/lib.rs @@ -4,8 +4,7 @@ #![feature(abi_x86_interrupt, try_blocks)] #![no_std] -use core::sync::atomic::Ordering; -use task::{KillReason, TaskRef}; +use task::KillReason; use x86_64::structures::idt::InterruptStackFrame; pub fn set_trap_flag(stack_frame: &mut InterruptStackFrame) {