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

[Benno Lossin]

On 11.09.24 16:50, Danilo Krummrich wrote:
> On Wed, Sep 11, 2024 at 03:27:57PM +0200, Alice Ryhl wrote:
>> On Wed, Sep 11, 2024 at 3:26 PM Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
>>> On 11.09.24 13:02, Danilo Krummrich wrote:
>>>> On Wed, Sep 11, 2024 at 08:36:38AM +0000, Benno Lossin wrote:
>>>>> 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:
>>>>>>>>>> +///
>>>>>>>>>> +/// ```
>>>>>>>>>> +/// # 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.
>>>>
>>>> Well, my point is how do we know? There's no language specification and the
>>>> documentation is (at least) ambiguous.
>>>
>>> So I went through the unsafe-coding-guidelines issues list and only
>>> found this one: https://github.com/rust-lang/unsafe-code-guidelines/issues/93
>>> Maybe I missed something. You could also try to ask at the rust zulip in
>>> the t-opsem channel for further clarification.
>>>
>>> I think we should just be on the safe side and assume that ZSTs require
>>> alignment. But if you get a convincing answer and if they say that they
>>> will document it, then I don't mind removing the alignment requirement.
> 
> I agree -- I also wrote this in a previous mail.
> 
> I was just wondering why you are so sure about it, since the documentation
> doesn't seem to be clear about it.

As Alice found below, the documentation is actually clear about this. (I
think I read it at some point, but forgot exactly where it was)

Maybe it could be better documented that dereferencing has the same
requirements as `read` (or whatever they are).

>> Please see the section on alignment in the same page. Just because a
>> pointer is valid does not mean that it is properly aligned.
>>
>> From the page:
>>
>> Valid raw pointers as defined above are not necessarily properly
>> aligned (where “proper” alignment is defined by the pointee type,
>> i.e., *const T must be aligned to mem::align_of::<T>()). However, most
>> functions require their arguments to be properly aligned, and will
>> explicitly state this requirement in their documentation. Notable
>> exceptions to this are read_unaligned and write_unaligned.
>>
>> When a function requires proper alignment, it does so even if the
>> access has size 0, i.e., even if memory is not actually touched.
>> Consider using NonNull::dangling in such cases.
> 
> Good point.
> 
> It still sounds like it's only required for functions that explicitly state so.
> 
> And as cited from nomicon "This is possibly needless pedantry because ptr::read
> is a noop for a ZST, [...]". But, no question, of course we have to honor it
> anyways.

This sounds to me like an implementation detail note, not something that
a caller should consider. But that's my interpretation.

---
Cheers,
Benno