linux-kernel - Re: [PATCH] rust: extend kbox with a new constructor

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: <DBWGKO6W6BK3.1ZTUQ8EI9LFJ7@kernel.org>
Date: Thu, 07 Aug 2025 21:56:09 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Vitaly Wool" <vitaly.wool@...sulko.se>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 "Uladzislau Rezki" <urezki@...il.com>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Vlastimil Babka" <vbabka@...e.cz>, "Lorenzo Stoakes"
 <lorenzo.stoakes@...cle.com>, "Liam R . Howlett" <Liam.Howlett@...cle.com>,
 "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>,
 "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>, "Bjorn
 Roy Baron" <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Trevor Gross"
 <tmgross@...ch.edu>
Subject: Re: [PATCH] rust: extend kbox with a new constructor

On Thu Aug 7, 2025 at 2:10 PM CEST, Vitaly Wool wrote:

Please start the patch subject with "rust: alloc:" and make the subject more
concrete, i.e. name the constructor you add, e.g. "rust: alloc: implement
Box::pin_slice()".

This makes things much more obvious when using 'git log --oneline'.

> From: Alice Ryhl <aliceryhl@...gle.com>
>
> Add a new constructor to KBox to facilitate KBox creation from a

NIT: You're adding it for all allorcators, hence "Box".

> pinned slice of elements. This allows to efficiently allocate memory for
> e.g. arrays of structrures containing spinlocks or mutexes.

Sounds reasonable, can you please mention where this will be used?

> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> Signed-off-by: Vitaly Wool <vitaly.wool@...sulko.se>
> ---
>  rust/kernel/alloc/kbox.rs | 51 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> index 1fef9beb57c8..74877afab0a3 100644
> --- a/rust/kernel/alloc/kbox.rs
> +++ b/rust/kernel/alloc/kbox.rs
> @@ -290,6 +290,57 @@ pub fn pin(x: T, flags: Flags) -> Result<Pin<Box<T, A>>, AllocError>
>          Ok(Self::new(x, flags)?.into())
>      }
>  
> +    /// Construct a pinned slice of elements `Pin<Box<[T], A>>`. This is a convenient means for
> +    /// creation of e.g. arrays of structrures containing spinlocks or mutexes.

Please add an empty line after the first sentence.

NIT: "slices of structures"

> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// #[pin_data]
> +    /// struct Example {
> +    ///     c: u32,
> +    ///     #[pin]
> +    ///     d: SpinLock<Inner>,
> +    /// }
> +    ///
> +    /// impl Example {
> +    ///     fn new() -> impl PinInit<Self> {
> +    ///         pin_init!(Self {
> +    ///             c: 10,
> +    ///             d <- new_spinlock!(Inner { a: 20, b: 30 }),
> +    ///         })
> +    ///     }
> +    /// }
> +    /// // Allocate a boxed slice of 10 `Example`s.
> +    /// let s = KBox::pin_slice(
> +    ///     | _i | Example::new(),
> +    ///     10,
> +    ///     GFP_KERNEL
> +    /// )?;

How would a more complex example look like where the slice items are not
identical, i.e. the impl PinInit<T, E> objects returned by the FnMut?

Do we need a temporary Vec for this? If so, it would probably make more sense to
have the following signature.

	pub fn pin_slice<F, I, E, Arg>(mut init: F, &[Arg], flags: Flags) -> Result<Pin<Box<[T], A>>, E>
	where
	   F: FnMut(&Arg) -> I,
	   I: PinInit<T, E>,
	   E: From<AllocError>,


Where Arg is some generic type containing the arguments passed to the closure
to create the impl PinInit<T, E>. For the example above Args could just be ().

> +    /// assert_eq!(s[5].c, 10);
> +    /// assert_eq!(s[3].d.lock().a, 20),
> +    /// ```
> +    pub fn pin_slice<F, I, E>(mut init: F, len: usize, flags: Flags) -> Result<Pin<Box<[T], A>>, E>

That's a lot of generics, we should probably consider longer names, e.g.
<Fn, Item, E>, where E is probably obvious enough.

> +    where
> +        F: FnMut(usize) -> I,
> +        I: PinInit<T, E>,
> +        E: From<AllocError>,
> +    {
> +        let mut buffer = super::Vec::<T, A>::with_capacity(len, flags)?;
> +        for i in 0..len {
> +            let ptr = buffer.spare_capacity_mut().as_mut_ptr().cast();
> +            // SAFETY: This address is available to be initialized, and it will either be dropped
> +            // on a future error or returned as a pinned location.

I think this should be a bit closer to

    // - `ptr` is a valid pointer to uninitialized memory.
    // - `ptr` is not used if an error is returned.
    // - `ptr` won't be moved until it is dropped, i.e. it is pinned.

> +            unsafe { init(i).__pinned_init(ptr)? };
> +            // SAFETY: We initialized one more value.

The safety requirement says:

    /// - `additional` must be less than or equal to `self.capacity - self.len`.
    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.

Please explain that the first is covered by with_capacity(len, ...) while to
loop iterator only goes to len - 1 and the second by explaining that the loop
initializes items in ascending order.

> +            unsafe { buffer.inc_len(1) };
> +        }
> +        let (ptr, _, _) = buffer.into_raw_parts();
> +        let slice = core::ptr::slice_from_raw_parts_mut(ptr, len);
> +        // SAFETY: This memory holds a valid [T] allocated with the right allocator.

What's the right allocator? I think you want to say the same allocates as used
for the Vec::with_capacity() call.

> +        Ok(Pin::from(unsafe { Box::from_raw(slice) }))
> +    }