On 11.09.24 01:25, Danilo Krummrich wrote: > On Tue, Sep 10, 2024 at 07:49:42PM +0000, Benno Lossin wrote: >> On 10.09.24 19:40, Danilo Krummrich wrote: >>> On Sat, Aug 31, 2024 at 05:39:07AM +0000, Benno Lossin wrote: >>>> On 16.08.24 02:10, Danilo Krummrich wrote: >>>>> +/// # Examples >>>>> +/// >>>>> +/// ``` >>>>> +/// let b = KBox::<u64>::new(24_u64, GFP_KERNEL)?; >>>>> +/// >>>>> +/// assert_eq!(*b, 24_u64); >>>>> +/// # Ok::<(), Error>(()) >>>>> +/// ``` >>>>> +/// >>>>> +/// ``` >>>>> +/// # use kernel::bindings; >>>>> +/// const SIZE: usize = bindings::KMALLOC_MAX_SIZE as usize + 1; >>>>> +/// struct Huge([u8; SIZE]); >>>>> +/// >>>>> +/// assert!(KBox::<Huge>::new_uninit(GFP_KERNEL | __GFP_NOWARN).is_err()); >>>>> +/// ``` >>>> >>>> It would be nice if you could add something like "KBox can't handle big >>>> allocations:" above this example, so that people aren't confused why >>>> this example expects an error. >>> >>> I don't think that's needed, it's implied by >>> `SIZE == bindings::KMALLOC_MAX_SIZE + 1`. >>> >>> Surely, we could add it nevertheless, but it's not very precise to just say "big >>> allocations". And I think this isn't the place for lengthy explanations of >>> `Kmalloc` behavior. >> >> Fair point, nevertheless I find examples a bit more useful, when the >> intention behind them is not only given as code. >> >>>>> +/// >>>>> +/// ``` >>>>> +/// # use kernel::bindings; >>>>> +/// const SIZE: usize = bindings::KMALLOC_MAX_SIZE as usize + 1; >>>>> +/// struct Huge([u8; SIZE]); >>>>> +/// >>>>> +/// assert!(KVBox::<Huge>::new_uninit(GFP_KERNEL).is_ok()); >>>>> +/// ``` >>>> >>>> Similarly, you could then say above this one "Instead use either `VBox` >>>> or `KVBox`:" >>>> >>>>> +/// >>>>> +/// # Invariants >>>>> +/// >>>>> +/// The [`Box`]' pointer is always properly aligned and either points to memory allocated with `A` >>>> >>>> Please use `self.0` instead of "[`Box`]'". >>>> >>>>> +/// or, for zero-sized types, is a dangling pointer. >>>> >>>> Probably "dangling, well aligned pointer.". >>> >>> Does this add any value? For ZSTs everything is "well aligned", isn't it? >> >> ZSTs can have alignment and then unaligned pointers do exist for them >> (and dereferencing them is UB!): > > Where is this documented? The documentation says: > > "For operations of size zero, *every* pointer is valid, including the null > pointer. The following points are only concerned with non-zero-sized accesses." > [1] That's a good point, the documentation looks a bit outdated. I found this page in the nomicon: https://doc.rust-lang.org/nomicon/vec/vec-zsts.html The first iterator implementation has an alignment issue. (Nevertheless, that chapter of the nomicon is probably useful to you, since it goes over implementing `Vec`, but maybe you already saw it) > [1] https://doc.rust-lang.org/std/ptr/index.html Might be a good idea to improve/complain about this at the rust project. >> #[repr(align(64))] >> struct Token; >> >> fn main() { >> let t = 64 as *mut Token; >> let t = unsafe { t.read() }; // this is fine. >> let t = 4 as *mut Token; >> let t = unsafe { t.read() }; // this is UB, see below for miri's output >> } >> >> Miri complains: >> >> error: Undefined Behavior: accessing memory based on pointer with alignment 4, but alignment 64 is required >> --> src/main.rs:8:22 >> | >> 8 | let t = unsafe { t.read() }; // this is UB, see below for miri's output >> | ^^^^^^^^ accessing memory based on pointer with alignment 4, but alignment 64 is required >> | >> = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior >> = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information >> = note: BACKTRACE: >> = note: inside `main` at src/main.rs:8:22: 8:30 > > `read` explicitly asks for non-null and properly aligned even if `T` has size > zero. Dereferencing (ie `*t`) also requires that (I just didn't do it, since then the `Token` must implement `Copy`). --- Cheers, Benno
