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

[Alice Ryhl]

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.

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.

Alice