linux-kernel - Re: [PATCH v5 3/7] rust: alloc: add Vec::push_within

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: <D9PWBFMQRL0P.1UGCE5V5SSBLZ@kernel.org>
Date: Wed, 07 May 2025 13:35:46 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Alice Ryhl" <aliceryhl@...gle.com>, "Danilo Krummrich"
 <dakr@...nel.org>
Cc: "Matthew Maurer" <mmaurer@...gle.com>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 3/7] rust: alloc: add Vec::push_within_capacity

On Fri May 2, 2025 at 3:19 PM CEST, Alice Ryhl wrote:
> This introduces a new method called `push_within_capacity` for appending
> to a vector without attempting to allocate if the capacity is full. Rust
> Binder will use this in various places to safely push to a vector while
> holding a spinlock.
>
> The implementation is moved to a push_within_capacity_unchecked method.
> This is preferred over having push() call push_within_capacity()
> followed by an unwrap_unchecked() for simpler unsafe.
>
> Panics in the kernel are best avoided when possible, so an error is
> returned if the vector does not have sufficient capacity. An error type
> is used rather than just returning Result<(),T> to make it more
> convenient for callers (i.e. they can use ? or unwrap).
>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>

One small nit below, with or without:

Reviewed-by: Benno Lossin <lossin@...nel.org>

[...]

> +    /// Appends an element to the back of the [`Vec`] instance without reallocating.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The length must be less than the capacity.

I would have written:

    - `self.len() < self.capacity()`

---
Cheers,
Benno

> +    pub unsafe fn push_within_capacity_unchecked(&mut self, v: T) {
>          let spare = self.spare_capacity_mut();
>  
> -        // SAFETY: The call to `reserve` was successful so the spare capacity is at least 1.
> +        // SAFETY: By the safety requirements, `spare` is non-empty.
>          unsafe { spare.get_unchecked_mut(0) }.write(v);
>  
>          // SAFETY: We just initialised the first spare entry, so it is safe to increase the length
> -        // by 1. We also know that the new length is <= capacity because of the previous call to
> -        // `reserve` above.
> +        // by 1. We also know that the new length is <= capacity because the caller guarantees that
> +        // the length is less than the capacity at the beginning of this function.
>          unsafe { self.inc_len(1) };
> -        Ok(())
>      }
>  
>      /// Removes the last element from a vector and returns it, or `None` if it is empty.