linux-kernel - Re: [PATCH 2/2] rust: block/mq: replace mem::zeroed() with Zeroable trait

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAH5fLgi9+d6PwZdU23xcF0p+m6mja3Dx8BufiiM-ZgLtDSqQDw@mail.gmail.com>
Date: Fri, 29 Nov 2024 10:41:53 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Paolo Bonzini <pbonzini@...hat.com>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, 
	boqun.feng@...il.com, ojeda@...nel.org, benno.lossin@...ton.me, 
	axboe@...nel.dk, tmgross@...ch.edu, bjorn3_gh@...tonmail.com, 
	gary@...yguo.net, alex.gaynor@...il.com, a.hindborg@...nel.org
Subject: Re: [PATCH 2/2] rust: block/mq: replace mem::zeroed() with Zeroable trait

On Thu, Nov 28, 2024 at 3:13 PM Paolo Bonzini <pbonzini@...hat.com> wrote:
>
> Isolate the unsafety in the declaration of the Zeroable trait, instead of having
> to use "unsafe" just to declare a struct.  This is more similar to how you would
> use "..Default::default()" (which is also a possibility here, but arguably
> less efficient).
>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
>  rust/kernel/block/mq/gen_disk.rs |  8 +++++---
>  rust/kernel/block/mq/tag_set.rs  | 10 ++++++----
>  2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
> index 708125dce96a..65342d065296 100644
> --- a/rust/kernel/block/mq/gen_disk.rs
> +++ b/rust/kernel/block/mq/gen_disk.rs
> @@ -6,7 +6,7 @@
>  //! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
>
>  use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
> -use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc};
> +use crate::{bindings, error::from_err_ptr, error::Result, init::Zeroable, sync::Arc};
>  use crate::{error, static_lock_class};
>  use core::fmt::{self, Write};
>
> @@ -31,6 +31,9 @@ fn default() -> Self {
>      }
>  }
>
> +// SAFETY: `bindings::queue_limits` contains only fields that are valid when zeroed.
> +unsafe impl Zeroable for bindings::queue_limits {}
> +
>  impl GenDiskBuilder {
>      /// Create a new instance.
>      pub fn new() -> Self {
> @@ -93,8 +96,7 @@ pub fn build<T: Operations>(
>          name: fmt::Arguments<'_>,
>          tagset: Arc<TagSet<T>>,
>      ) -> Result<GenDisk<T>> {
> -        // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
> -        let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
> +        let mut lim: bindings::queue_limits = Zeroable::ZERO;
>
>          lim.logical_block_size = self.logical_block_size;
>          lim.physical_block_size = self.physical_block_size;
> diff --git a/rust/kernel/block/mq/tag_set.rs b/rust/kernel/block/mq/tag_set.rs
> index f9a1ca655a35..1ff7366ca549 100644
> --- a/rust/kernel/block/mq/tag_set.rs
> +++ b/rust/kernel/block/mq/tag_set.rs
> @@ -10,6 +10,7 @@
>      bindings,
>      block::mq::{operations::OperationsVTable, request::RequestDataWrapper, Operations},
>      error,
> +    init::Zeroable,
>      prelude::PinInit,
>      try_pin_init,
>      types::Opaque,
> @@ -32,6 +33,10 @@ pub struct TagSet<T: Operations> {
>      _p: PhantomData<T>,
>  }
>
> +// SAFETY: `blk_mq_tag_set` only contains integers and pointers, which
> +// all are allowed to be 0.
> +unsafe impl Zeroable for bindings::blk_mq_tag_set {}

This will have to be reverted if we want to split up the kernel crate
due to the orphan rule.

Alice