linux-kernel - Re: [PATCH v5 09/26] rust: alloc: implement kernel `Box`

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zr4J4e1aLADlyDMD@cassiopeiae>
Date: Thu, 15 Aug 2024 16:00:01 +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 v5 09/26] rust: alloc: implement kernel `Box`

On Thu, Aug 15, 2024 at 01:24:47PM +0000, Benno Lossin wrote:
> On 14.08.24 23:58, Danilo Krummrich wrote:
> > On Wed, Aug 14, 2024 at 05:01:34PM +0000, Benno Lossin wrote:
> >> On 12.08.24 20:22, Danilo Krummrich wrote:
> >>> +/// The kernel's [`Box`] type - a heap allocation for a single value of type `T`.
> >>> +///
> >>> +/// This is the kernel's version of the Rust stdlib's `Box`. There are a couple of differences,
> >>> +/// for example no `noalias` attribute is emitted and partially moving out of a `Box` is not
> >>> +/// supported.
> >>
> >> I would add "But otherwise it works the same." (I don't know if there is
> >> a comma needed after the "otherwise").
> > 
> > There are more differences we don't list here, and probably don't need to.
> > Hence, saying that it otherwise works the same isn't correct.
> > 
> >> Also I remember that there was one more difference with a custom box
> >> compared to the stdlib, but I forgot what that was, does someone else
> >> remember? We should also put that here.
> > 
> > Obviously, there are also quite some API differences. For instance, `Box`
> > generally requires two generics, value type and allocator, we take page flags
> > and return a `Result`, where std just panics on failure.
> 
> Oh yeah that's true. The things listed above don't really refer to API
> stuff, so I didn't consider that. How about changing "couple
> differences" to "several differences"? Also adding that the APIs are
> different would not hurt.
> 

Sure.

> 
> >>> +
> >>> +/// Type alias for `Box` with a `Kmalloc` allocator.
> >>
> >> I think we should add that this is only designed for small values.
> > 
> > I don't want duplicate the existing documentation around kmalloc and friends
> > [1].
> > 
> > Maybe we can refer to the existing documentation somehow.
> > 
> > [1] https://www.kernel.org/doc/html/latest/core-api/memory-allocation.html
> 
> Oh great! With the C docs, I never know where to find them (is it in the
> code and do they exist?). Yeah let's just link it.
> 
> >>> +///
> >>> +/// # Examples
> >>> +///
> >>> +/// ```
> >>> +/// let b = KBox::new(24_u64, GFP_KERNEL)?;
> >>> +///
> >>> +/// assert_eq!(*b, 24_u64);
> >>> +///
> >>> +/// # Ok::<(), Error>(())
> >>> +/// ```
> >>> +pub type KBox<T> = Box<T, super::allocator::Kmalloc>;
> >>> +
> >>> +/// Type alias for `Box` with a `Vmalloc` allocator.
> >>
> >> Same here, add that this is supposed to be used for big values (or is
> >> this also a general-purpose allocator, just not guaranteeing that the
> >> memory is physically contiguous? in that case I would document it
> >> here and also on `Vmalloc`).
> > 
> > Same as above, I'd rather not duplicate that. But I'm happy to link things in,
> > just not sure what's the best way doing it.
> 
> I took a look at the link and there is the "Selecting memory allocator"
> section, but there isn't really just a vmalloc or kmalloc section, it is
> rather stuff that we would put in the module documentation.

There are no dedicated sections, but...

> What I would write on these types would be what to use these boxes for.
> eg large allocations, general purpose etc. I don't think that that is
> easily accessible from the docs that you linked above.

...this stuff should be covered by the document, e.g.:

"The maximal size of a chunk that can be allocated with kmalloc is limited. The
actual limit depends on the hardware and the kernel configuration, but it is a
good practice to use kmalloc for objects smaller than page size."

or

"For large allocations you can use vmalloc() and vzalloc(), or directly request
pages from the page allocator. The memory allocated by vmalloc and related
functions is not physically contiguous."

I'd probably reference [1] in the module documentation as you say and otherwise
refer to the C APIs, just like `RBTree` does.

> 
> ---
> Cheers,
> Benno
>