Re: [PATCH v6 09/26] rust: alloc: implement kernel `Box`

[Benno Lossin]

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