Message-ID: <ZuF444VO8gK8l7UR@pollux>
Date: Wed, 11 Sep 2024 13:02:59 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com,
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	a.hindborg@...sung.com, aliceryhl@...gle.com,
	akpm@...ux-foundation.org, daniel.almeida@...labora.com,
	faith.ekstrand@...labora.com, boris.brezillon@...labora.com,
	lina@...hilina.net, mcanal@...lia.com, zhiw@...dia.com,
	cjia@...dia.com, jhubbard@...dia.com, airlied@...hat.com,
	ajanulgu@...hat.com, lyude@...hat.com, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v6 09/26] rust: alloc: implement kernel `Box`

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:
> >>>>> +/// # 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.

Well, my point is how do we know? There's no language specification and the
documentation is (at least) ambiguous.

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

I mentioned this because for `read` it's explicitly documented.

However, the nomicon also says "This is possibly needless pedantry because
ptr::read is a noop for a ZST, [...]".

> 
> Dereferencing (ie `*t`) also requires that (I just didn't do it, since
> then the `Token` must implement `Copy`).

Again, how do you know? The documentation isn't clear about it.

> 
> ---
> Cheers,
> Benno
> 

Powered by blists - more mailing lists