linux-kernel - Re: [PATCH v6 13/26] rust: alloc: implement kernel `Vec` type

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: <ZtdemCduZ1KqayXw@boqun-archlinux>
Date: Tue, 3 Sep 2024 12:08:08 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com,
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	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 13/26] rust: alloc: implement kernel `Vec` type

On Fri, Aug 16, 2024 at 02:10:55AM +0200, Danilo Krummrich wrote:
> `Vec` provides a contiguous growable array type (such as `Vec`) with
> contents allocated with the kernel's allocators (e.g. `Kmalloc`,
> `Vmalloc` or `KVmalloc`).
> 
> In contrast to Rust's `Vec` type, the kernel `Vec` type considers the
> kernel's GFP flags for all appropriate functions, always reports
> allocation failures through `Result<_, AllocError>` and remains
> independent from unstable features.
> 
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> ---
[...]
> +
> +impl<T, A> Vec<T, A>
> +where
> +    A: Allocator,
> +{
[...]
> +    /// Forcefully sets `self.len` to `new_len`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `new_len` must be less than or equal to [`Self::capacity`].
> +    /// - If `new_len` is greater than `self.len`, all elements within the interval
> +    ///   [`self.len`,`new_len`] must be initialized.

Maybe use "[`self.len`, `new_len`)" to indicate `new_len` side is open?
Also `self.len` may confuse people whether it's the old length or new
length, could you use `old_len` and add note saying "`old_len` is the
length before `set_len()`?

> +    #[inline]
> +    pub unsafe fn set_len(&mut self, new_len: usize) {
> +        self.len = new_len;
> +    }
> +
> +    /// Returns a slice of the entire vector.
> +    ///
> +    /// Equivalent to `&s[..]`.

"s" is used here without pre-definition, it's a bit confusing. Either we
use an example here, or we can just avoid mentioning it at all?

> +    #[inline]
> +    pub fn as_slice(&self) -> &[T] {
> +        self
> +    }
> +
> +    /// Returns a mutable slice of the entire vector.
> +    ///
> +    /// Equivalent to `&mut s[..]`.

Ditto.

> +    #[inline]
> +    pub fn as_mut_slice(&mut self) -> &mut [T] {
> +        self
> +    }
> +
> +    /// Returns a mutable raw pointer to the vector's backing buffer, or, if `T` is a ZST, a
> +    /// dangling raw pointer.
> +    #[inline]
> +    pub fn as_mut_ptr(&self) -> *mut T {

I think this function needs to take a `&mut self` to mirror the
`as_mut_ptr` of a slice.

Regards,
Boqun

> +        self.ptr.as_ptr()
> +    }
> +
> +    /// Returns a raw pointer to the vector's backing buffer, or, if `T` is a ZST, a dangling raw
> +    /// pointer.
> +    #[inline]
> +    pub fn as_ptr(&self) -> *const T {
> +        self.as_mut_ptr()
> +    }
> +
[...]