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!):
#[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
>>> +#[repr(transparent)]
>>> +pub struct Box<T: ?Sized, A: Allocator>(NonNull<T>, PhantomData<A>);
>>> +impl<T, A> Box<T, A>
>>> +where
>>> + T: ?Sized,
>>> + A: Allocator,
>>> +{
>>> + /// Creates a new `Box<T, A>` from a raw pointer.
>>> + ///
>>> + /// # Safety
>>> + ///
>>> + /// For non-ZSTs, `raw` must point at an allocation allocated with `A`that is sufficiently
>>> + /// aligned for and holds a valid `T`. The caller passes ownership of the allocation to the
>>> + /// `Box`.
>>
>> You don't say what must happen for ZSTs.
>
> Because we don't require anything for a ZST, do we?
We require a non-null, well aligned pointer (see above).
---
Cheers,
Benno
>>> + #[inline]
>>> + pub const unsafe fn from_raw(raw: *mut T) -> Self {
>>> + // INVARIANT: Validity of `raw` is guaranteed by the safety preconditions of this function.
>>> + // SAFETY: By the safety preconditions of this function, `raw` is not a NULL pointer.
>>> + Self(unsafe { NonNull::new_unchecked(raw) }, PhantomData::<A>)
>>> + }
