linux-kernel - Re: [PATCH v2 05/14] rust: block: use `NullBorrowFormatter`

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] [day] [month] [year] [list]
Message-ID: <CAH5fLgjwmXJP2LcE8UKP1gdqbsTA5QyynLcw5iG93hXeLuOBAA@mail.gmail.com>
Date: Fri, 11 Jul 2025 12:02:19 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: Boqun Feng <boqun.feng@...il.com>, Miguel Ojeda <ojeda@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <lossin@...nel.org>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, Jens Axboe <axboe@...nel.dk>, linux-block@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 05/14] rust: block: use `NullBorrowFormatter`

On Fri, Jul 11, 2025 at 11:29 AM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>
> "Alice Ryhl" <aliceryhl@...gle.com> writes:
>
> > On Tue, Jul 08, 2025 at 09:45:00PM +0200, Andreas Hindborg wrote:
> >> Use the new `NullBorrowFormatter` to write the name of a `GenDisk` to the
> >> name buffer. This new formatter automatically adds a trailing null marker
> >> after the written characters, so we don't need to append that at the call
> >> site any longer.
> >>
> >> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
> >> ---
> >>  rust/kernel/block/mq/gen_disk.rs   | 8 ++++----
> >>  rust/kernel/block/mq/raw_writer.rs | 1 +
> >>  rust/kernel/str.rs                 | 7 -------
> >>  3 files changed, 5 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
> >> index 679ee1bb21950..e0e42f7028276 100644
> >> --- a/rust/kernel/block/mq/gen_disk.rs
> >> +++ b/rust/kernel/block/mq/gen_disk.rs
> >> @@ -7,9 +7,10 @@
> >>
> >>  use crate::{
> >>      bindings,
> >> -    block::mq::{raw_writer::RawWriter, Operations, TagSet},
> >> +    block::mq::{Operations, TagSet},
> >>      error::{self, from_err_ptr, Result},
> >>      static_lock_class,
> >> +    str::NullBorrowFormatter,
> >>      sync::Arc,
> >>  };
> >>  use core::fmt::{self, Write};
> >> @@ -143,14 +144,13 @@ pub fn build<T: Operations>(
> >>          // SAFETY: `gendisk` is a valid pointer as we initialized it above
> >>          unsafe { (*gendisk).fops = &TABLE };
> >>
> >> -        let mut raw_writer = RawWriter::from_array(
> >> +        let mut writer = NullBorrowFormatter::from_array(
> >>              // SAFETY: `gendisk` points to a valid and initialized instance. We
> >>              // have exclusive access, since the disk is not added to the VFS
> >>              // yet.
> >>              unsafe { &mut (*gendisk).disk_name },
> >>          )?;
> >> -        raw_writer.write_fmt(name)?;
> >> -        raw_writer.write_char('\0')?;
> >> +        writer.write_fmt(name)?;
> >
> > Although this is nicer than the existing code, I wonder if it should
> > just be a function rather than a whole NullBorrowFormatter struct? Take
> > a slice and a fmt::Arguments and write it with a nul-terminator. Do you
> > need anything more complex than what you have here?
>
> I don't need anything more complex right now. But I think the
> `NullTerminatedFormatter` could be useful anyway:
>
>   +/// A mutable reference to a byte buffer where a string can be written into.
>   +///
>   +/// The buffer will be automatically null terminated after the last written character.
>   +///
>   +/// # Invariants
>   +///
>   +/// `buffer` is always null terminated.
>   +pub(crate) struct NullTerminatedFormatter<'a> {
>   +    buffer: &'a mut [u8],
>   +}
>   +
>   +impl<'a> NullTerminatedFormatter<'a> {
>   +    /// Create a new [`Self`] instance.
>   +    pub(crate) fn new(buffer: &'a mut [u8]) -> Option<NullTerminatedFormatter<'a>> {
>   +        *(buffer.first_mut()?) = 0;
>   +
>   +        // INVARIANT: We null terminated the buffer above.
>   +        Some(Self { buffer })
>   +    }
>   +
>   +    pub(crate) fn from_array<const N: usize>(
>   +        buffer: &'a mut [crate::ffi::c_char; N],
>   +    ) -> Option<NullTerminatedFormatter<'a>> {
>   +        Self::new(buffer)
>   +    }
>   +}
>   +
>   +impl Write for NullTerminatedFormatter<'_> {
>   +    fn write_str(&mut self, s: &str) -> fmt::Result {
>   +        let bytes = s.as_bytes();
>   +        let len = bytes.len();
>   +
>   +        // We want space for a null terminator. Buffer length is always at least 1, so no overflow.
>   +        if len > self.buffer.len() - 1 {
>   +            return Err(fmt::Error);
>   +        }
>   +
>   +        let buffer = core::mem::take(&mut self.buffer);
>   +        // We break the null termination invariant for a short while.
>   +        buffer[..len].copy_from_slice(bytes);
>   +        self.buffer = &mut buffer[len..];
>   +
>   +        // INVARIANT: We null terminate the buffer.
>   +        self.buffer[0] = 0;
>   +
>   +        Ok(())
>   +    }
>   +}
>   +
>
> If you insist, I can write something like
>
>   fn format_to_buffer(buffer: &mut [u8], args: fmt::Arguments) -> fmt::Result
>
> although I am not sure I see the point of this change.

I don't mind. I just thought it was simpler since you only need to
support a single write instead of having to support multiple writes.

Alice