From a1f81df26383046deb9f9da3d1de4c714d27fce1 Mon Sep 17 00:00:00 2001 From: Ruihan Li Date: Wed, 19 Mar 2025 13:36:23 +0800 Subject: [PATCH] Remove `ostd/src/mm/offset.rs` --- .../comps/virtio/src/device/input/device.rs | 1 - .../comps/virtio/src/device/socket/device.rs | 2 +- kernel/comps/virtio/src/queue.rs | 8 +-- .../comps/virtio/src/transport/mmio/device.rs | 9 ++- .../comps/virtio/src/transport/pci/device.rs | 1 - kernel/libs/aster-util/src/safe_ptr.rs | 16 +++-- kernel/libs/aster-util/src/union_read_ptr.rs | 51 +++++++------- kernel/src/process/signal/c_types.rs | 15 ++-- ostd/src/mm/mod.rs | 1 - ostd/src/mm/offset.rs | 69 ------------------- ostd/src/util/macros.rs | 32 +++++++++ 11 files changed, 82 insertions(+), 123 deletions(-) delete mode 100644 ostd/src/mm/offset.rs diff --git a/kernel/comps/virtio/src/device/input/device.rs b/kernel/comps/virtio/src/device/input/device.rs index 277fb207..1e6e293d 100644 --- a/kernel/comps/virtio/src/device/input/device.rs +++ b/kernel/comps/virtio/src/device/input/device.rs @@ -18,7 +18,6 @@ use log::{debug, info}; use ostd::{ io::IoMem, mm::{DmaDirection, DmaStream, FrameAllocOptions, HasDaddr, VmIo, PAGE_SIZE}, - offset_of, sync::{LocalIrqDisabled, RwLock, SpinLock}, trap::TrapFrame, }; diff --git a/kernel/comps/virtio/src/device/socket/device.rs b/kernel/comps/virtio/src/device/socket/device.rs index 4170b620..9eaae932 100644 --- a/kernel/comps/virtio/src/device/socket/device.rs +++ b/kernel/comps/virtio/src/device/socket/device.rs @@ -6,7 +6,7 @@ use core::{fmt::Debug, hint::spin_loop, mem::size_of}; use aster_network::{RxBuffer, TxBuffer}; use aster_util::{field_ptr, slot_vec::SlotVec}; use log::debug; -use ostd::{mm::VmWriter, offset_of, sync::SpinLock, trap::TrapFrame, Pod}; +use ostd::{mm::VmWriter, sync::SpinLock, trap::TrapFrame, Pod}; use super::{ config::{VirtioVsockConfig, VsockFeatures}, diff --git a/kernel/comps/virtio/src/queue.rs b/kernel/comps/virtio/src/queue.rs index 2957ad53..213ba04b 100644 --- a/kernel/comps/virtio/src/queue.rs +++ b/kernel/comps/virtio/src/queue.rs @@ -4,7 +4,7 @@ use alloc::vec::Vec; use core::{ - mem::size_of, + mem::{offset_of, size_of}, sync::atomic::{fence, Ordering}, }; @@ -14,7 +14,7 @@ use bitflags::bitflags; use log::debug; use ostd::{ mm::{DmaCoherent, FrameAllocOptions, PodOnce}, - offset_of, Pod, + Pod, }; use crate::{ @@ -309,7 +309,7 @@ impl VirtQueue { let last_used_slot = self.last_used_idx & (self.queue_size - 1); let element_ptr = { let mut ptr = self.used.borrow_vm(); - ptr.byte_add(offset_of!(UsedRing, ring) as usize + last_used_slot as usize * 8); + ptr.byte_add(offset_of!(UsedRing, ring) + last_used_slot as usize * 8); ptr.cast::() }; let index = field_ptr!(&element_ptr, UsedElem, id).read_once().unwrap(); @@ -333,7 +333,7 @@ impl VirtQueue { let last_used_slot = self.last_used_idx & (self.queue_size - 1); let element_ptr = { let mut ptr = self.used.borrow_vm(); - ptr.byte_add(offset_of!(UsedRing, ring) as usize + last_used_slot as usize * 8); + ptr.byte_add(offset_of!(UsedRing, ring) + last_used_slot as usize * 8); ptr.cast::() }; let index = field_ptr!(&element_ptr, UsedElem, id).read_once().unwrap(); diff --git a/kernel/comps/virtio/src/transport/mmio/device.rs b/kernel/comps/virtio/src/transport/mmio/device.rs index c3a42ee3..12c1297d 100644 --- a/kernel/comps/virtio/src/transport/mmio/device.rs +++ b/kernel/comps/virtio/src/transport/mmio/device.rs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MPL-2.0 use alloc::{boxed::Box, sync::Arc}; -use core::mem::size_of; +use core::mem::{offset_of, size_of}; use aster_rights::{ReadOp, WriteOp}; use aster_util::{field_ptr, safe_ptr::SafePtr}; @@ -16,7 +16,6 @@ use ostd::{ }, io::IoMem, mm::{DmaCoherent, PAGE_SIZE}, - offset_of, sync::RwLock, trap::IrqCallbackFunction, }; @@ -66,9 +65,9 @@ impl VirtioMmioTransport { let interrupt_ack_offset = offset_of!(VirtioMmioLayout, interrupt_ack); let interrupt_status_offset = offset_of!(VirtioMmioLayout, interrupt_status); let mut interrupt_ack = layout.clone(); - interrupt_ack.byte_add(interrupt_ack_offset as usize); + interrupt_ack.byte_add(interrupt_ack_offset); let mut interrupt_status = layout.clone(); - interrupt_status.byte_add(interrupt_status_offset as usize); + interrupt_status.byte_add(interrupt_status_offset); ( interrupt_ack.cast::().restrict::(), interrupt_status.cast::().restrict::(), @@ -174,7 +173,7 @@ impl VirtioTransport for VirtioMmioTransport { } fn notify_config(&self, _idx: usize) -> ConfigManager { - let offset = offset_of!(VirtioMmioLayout, queue_notify) as usize; + let offset = offset_of!(VirtioMmioLayout, queue_notify); let safe_ptr = Some(SafePtr::new(self.common_device.io_mem().clone(), offset)); ConfigManager::new(safe_ptr, None) diff --git a/kernel/comps/virtio/src/transport/pci/device.rs b/kernel/comps/virtio/src/transport/pci/device.rs index 73d01abe..9e30e0dc 100644 --- a/kernel/comps/virtio/src/transport/pci/device.rs +++ b/kernel/comps/virtio/src/transport/pci/device.rs @@ -15,7 +15,6 @@ use ostd::{ }, io::IoMem, mm::DmaCoherent, - offset_of, trap::IrqCallbackFunction, }; diff --git a/kernel/libs/aster-util/src/safe_ptr.rs b/kernel/libs/aster-util/src/safe_ptr.rs index 59416912..4c76f74a 100644 --- a/kernel/libs/aster-util/src/safe_ptr.rs +++ b/kernel/libs/aster-util/src/safe_ptr.rs @@ -410,21 +410,27 @@ impl Debug for SafePtr { #[macro_export] macro_rules! field_ptr { ($ptr:expr, $type:ty, $($field:tt)+) => {{ - use ostd::offset_of; use aster_util::safe_ptr::SafePtr; #[inline] fn new_field_ptr( container_ptr: &SafePtr, - field_offset: *const U + field_offset: usize, + _type_infer: *const U, ) -> SafePtr { let mut ptr = container_ptr.borrow_vm(); - ptr.byte_add(field_offset as usize); + ptr.byte_add(field_offset); ptr.cast() } - let field_offset = offset_of!($type, $($field)*); - new_field_ptr($ptr, field_offset) + let field_offset = core::mem::offset_of!($type, $($field)*); + let type_infer = ostd::ptr_null_of!({ + let ptr: *const $type = core::ptr::null(); + // This is not sound, but the code won't be executed. + &raw const (*ptr).$($field)* + }); + + new_field_ptr($ptr, field_offset, type_infer) }} } diff --git a/kernel/libs/aster-util/src/union_read_ptr.rs b/kernel/libs/aster-util/src/union_read_ptr.rs index 30d3b2e5..bea6ad76 100644 --- a/kernel/libs/aster-util/src/union_read_ptr.rs +++ b/kernel/libs/aster-util/src/union_read_ptr.rs @@ -1,44 +1,39 @@ // SPDX-License-Identifier: MPL-2.0 -use core::marker::PhantomData; - use ostd::Pod; -/// This ptr is designed to read union field safely. -/// Write to union field is safe operation. While reading union field is UB. -/// However, if this field is Pod, we can safely read that field. -pub struct UnionReadPtr<'a, T: Pod> { +/// A reader to read `Pod` fields from a `Pod` type. +pub struct Reader<'a> { bytes: &'a [u8], - marker: PhantomData<&'a mut T>, } -impl<'a, T: Pod> UnionReadPtr<'a, T> { - pub fn new(object: &'a T) -> Self { - let bytes = object.as_bytes(); +impl<'a> Reader<'a> { + pub fn new(object: &'a T) -> Self { Self { - bytes, - marker: PhantomData, + bytes: object.as_bytes(), } } - pub fn read_at(&self, offset: *const F) -> F { - let offset = offset as usize; - F::from_bytes(&self.bytes[offset..]) + pub fn read_at(&self, field_offset: usize, _type_infer: *const F) -> F { + F::from_bytes(&self.bytes[field_offset..]) } } -/// FIXME: This macro requires the first argument to be a `reference` of a Pod type. -/// This is because the value_offset macro requires #[macro_export] -macro_rules! read_union_fields { - ($container:ident) => ({ - let offset = value_offset!($container); - let union_read_ptr = UnionReadPtr::new(&*$container); - union_read_ptr.read_at(offset) - }); - ($container:ident.$($field:ident).*) => ({ - let field_offset = ostd::value_offset!($container.$($field).*); - let union_read_ptr = UnionReadPtr::new(&*$container); - union_read_ptr.read_at(field_offset) - }); +macro_rules! read_union_field { + ($container:expr, $type:ty, $($field:tt)+) => {{ + use $crate::union_read_ptr::Reader; + + // Perform type checking first. + let container: &$type = $container; + let reader = Reader::new(container); + + let field_offset = core::mem::offset_of!($type, $($field)*); + let type_infer = ostd::ptr_null_of!({ + // This is not safe, but the code won't be executed. + &raw const container.$($field)* + }); + + reader.read_at(field_offset, type_infer) + }} } diff --git a/kernel/src/process/signal/c_types.rs b/kernel/src/process/signal/c_types.rs index e2e2afe8..d7b94a52 100644 --- a/kernel/src/process/signal/c_types.rs +++ b/kernel/src/process/signal/c_types.rs @@ -5,7 +5,7 @@ use core::mem::{self, size_of}; -use aster_util::{read_union_fields, union_read_ptr::UnionReadPtr}; +use aster_util::read_union_field; use super::sig_num::SigNum; use crate::{ @@ -55,8 +55,7 @@ impl siginfo_t { } pub fn si_addr(&self) -> Vaddr { - // let siginfo = *self; - read_union_fields!(self.siginfo_fields.sigfault.addr) + read_union_field!(self, Self, siginfo_fields.sigfault.addr) } } @@ -120,11 +119,11 @@ pub union sigval_t { impl sigval_t { pub fn read_int(&self) -> i32 { - read_union_fields!(self.sigval_int) + read_union_field!(self, Self, sigval_int) } pub fn read_ptr(&self) -> Vaddr { - read_union_fields!(self.sigval_ptr) + read_union_field!(self, Self, sigval_ptr) } } @@ -233,15 +232,15 @@ pub union _sigev_un { impl _sigev_un { pub fn read_tid(&self) -> i32 { - read_union_fields!(self._tid) + read_union_field!(self, Self, _tid) } pub fn read_function(&self) -> Vaddr { - read_union_fields!(self._sigev_thread.function) + read_union_field!(self, Self, _sigev_thread.function) } pub fn read_attribute(&self) -> Vaddr { - read_union_fields!(self._sigev_thread.attribute) + read_union_field!(self, Self, _sigev_thread.attribute) } } diff --git a/ostd/src/mm/mod.rs b/ostd/src/mm/mod.rs index 49a1bfb9..fa58c042 100644 --- a/ostd/src/mm/mod.rs +++ b/ostd/src/mm/mod.rs @@ -13,7 +13,6 @@ pub mod frame; pub mod heap; mod io; pub(crate) mod kspace; -mod offset; pub(crate) mod page_prop; pub(crate) mod page_table; pub mod tlb; diff --git a/ostd/src/mm/offset.rs b/ostd/src/mm/offset.rs deleted file mode 100644 index 50deb6d1..00000000 --- a/ostd/src/mm/offset.rs +++ /dev/null @@ -1,69 +0,0 @@ -// SPDX-License-Identifier: MPL-2.0 - -/// Gets the offset of a field within a type as a pointer. -/// -/// ```rust -/// #[repr(C)] -/// pub struct Foo { -/// first: u8, -/// second: u32, -/// } -/// -/// assert!(offset_of(Foo, first) == (0 as *const u8)); -/// assert!(offset_of(Foo, second) == (4 as *const u32)); -/// ``` -#[macro_export] -macro_rules! offset_of { - ($container:ty, $($field:tt)+) => ({ - // SAFETY: It is ok to have this uninitialized value because - // 1) Its memory won't be accessed; - // 2) It will be forgotten rather than being dropped; - // 3) Before it gets forgotten, the code won't return prematurely or panic. - let tmp: $container = unsafe { core::mem::MaybeUninit::uninit().assume_init() }; - - let container_addr = &tmp as *const _; - let field_addr = &tmp.$($field)* as *const _; - - #[expect(clippy::forget_non_drop)] - ::core::mem::forget(tmp); - - let field_offset = (field_addr as usize - container_addr as usize) as *const _; - - // Let Rust compiler infer our intended pointer type of field_offset - // by comparing it with another pointer. - let _: bool = field_offset == field_addr; - - field_offset - }); -} - -/// Gets the offset of a field within an object as a pointer. -/// -/// ```rust -/// #[repr(C)] -/// pub struct Foo { -/// first: u8, -/// second: u32, -/// } -/// let foo = &Foo {first: 0, second: 0}; -/// assert!(value_offset!(foo) == (0 as *const Foo)); -/// assert!(value_offset!(foo.first) == (0 as *const u8)); -/// assert!(value_offset!(foo.second) == (4 as *const u32)); -/// ``` -#[macro_export] -macro_rules! value_offset { - ($container:ident) => ({ - let container_addr = &*$container as *const _; - let offset = 0 as *const _; - let _: bool = offset == container_addr; - offset - }); - ($container:ident.$($field:ident).*) => ({ - let container_addr = &*$container as *const _; - // SAFETY: This is safe since we never access the field - let field_addr = unsafe {&($container.$($field).*)} as *const _; - let field_offset = (field_addr as usize- container_addr as usize) as *const _; - let _: bool = field_offset == field_addr; - field_offset - }); -} diff --git a/ostd/src/util/macros.rs b/ostd/src/util/macros.rs index ae6aa687..77db7dc2 100644 --- a/ostd/src/util/macros.rs +++ b/ostd/src/util/macros.rs @@ -14,3 +14,35 @@ macro_rules! const_assert { ($cond:expr $(,)?) => { const _: () = assert!($cond); }; ($cond:expr, $($arg:tt)+) => { const _: () = assert!($cond, $($arg)*); }; } + +/// Creates a pointer whose type matches the expression, but whose value is always NULL. +/// +/// This is a helper macro, typically used in another macro to help with type inference. +/// +/// The expression is guaranteed never to be executed, so it can contain arbitrarily unsafe code +/// without causing any soundness problems. +#[macro_export] +macro_rules! ptr_null_of { + ($expr:expr $(,)?) => { + if true { + core::ptr::null() + } else { + unreachable!(); + + // SAFETY: This is dead code and will never be executed. + // + // One may wonder: is it possible for the dead code to + // trigger UBs by simply being compiled, rather than being executed? + // More specifically, what if the caller attempts to + // trick the macro into defining unsafe language items, + // like static variables, functions, implementation blocks, or attributes, + // those that are not executed. + // Luckily for us, in such cases, the Rust compiler would complain that + // "items do not inherit unsafety from separate enclosing items". + #[expect(unreachable_code)] + unsafe { + $expr + } + } + }; +}