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

[Danilo Krummrich]

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.

> 
> 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.

> 
> Alice
>