[RFC PATCH 0/2] rust: Zeroable: allow struct update syntax outside init macros

[RFC PATCH 0/2] rust: Zeroable: allow struct update syntax outside init macros

[Paolo Bonzini]

The Zeroable trait is a marker trait, even though the various init macros
use a "fake" struct update syntax.  Sometimes, such a struct update
syntax can be useful even outside the init macros; therefore, this series
adds an associated const that returns an all-zero instance of a Zeroable type.

I'm sending this as RFC mostly because the diffstat is not too favorable.
This is mostly because patch 2 has to keep safety comments above the
"unsafe trait Zeroable" declarations.  It would be better if the trait
could be derived automatically, for example via "div rustbindgen" comments
(not my favorite syntax, and grossly underdocumented; but still).  That
would also remove "unsafe" altogether.

Nevertheless, it seems to me that this is a small improvement in
readability of the code that *uses* the structs, and it may be worth
considering it.

Another request for comments is whether the "..Zeroable::zeroed()" fake
struct update syntax used by the init macros should be changed to use
"..Zeroable::ZERO".  The trait does not reuse the init macro syntax,
because traits do not support const functions and it can be useful
to use Zeroable::ZERO in const context.

Personally I think it's not a problem, and decided to keep the two
spellings separate: "zeroed()" when working with the Init and PinInit
traits, and "ZERO" when working with the actual struct.  As far as I can
see, "..Zeroable::zeroed()" is unused in rust-dev, which makes it trivial
to switch.

Paolo


Paolo Bonzini (2):
  rust: Zeroable: allow struct update syntax outside init macros
  rust: block/mq: replace mem::zeroed() with Zeroable trait

 rust/kernel/block/mq/gen_disk.rs |  8 +++++---
 rust/kernel/block/mq/tag_set.rs  | 10 ++++++----
 rust/kernel/init.rs              |  7 ++++++-
 3 files changed, 18 insertions(+), 9 deletions(-)

-- 
2.47.0

[PATCH 1/2] rust: Zeroable: allow struct update syntax outside init macros

[Paolo Bonzini]

The Zeroable trait is a marker trait, even though the various init macros
use a "fake" struct update syntax.  Sometimes, such a struct update
syntax can be useful even outside the init macros.  Add an associated
const that returns an all-zero instance of a Zeroable type.

The exact syntax used by the init macros cannot be reproduced without
forgoing the ability to use Zeroable::ZERO in const context.  However,
it might not be a good idea to add a fn zeroed() inside the
Zeroable trait, to avoid confusion with the init::zeroed() function
and because Zeroable::ZERO is unrelated to the Init and PinInit
traits.  In other words, let's treat this difference as a
feature rather than a bug.

The definition of the ZERO constant requires adding a Sized boundary, but
this is not a problem either because neither slices nor trait objects
are zeroable.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/kernel/init.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletion(-)

diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index a17ac8762d8f..a00e7ff6a513 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -1392,7 +1392,12 @@ pub unsafe trait PinnedDrop: __internal::HasPinData {
 /// ```rust,ignore
 /// let val: Self = unsafe { core::mem::zeroed() };
 /// ```
-pub unsafe trait Zeroable {}
+pub unsafe trait Zeroable: Sized {
+    /// Return a value of Self whose memory representation consists of all zeroes.
+    // SAFETY: the Zeroable trait itself is unsafe, and declaring it (whether
+    // manually or via derivation) implies that this is not undefined behavior.
+    const ZERO: Self = unsafe { core::mem::zeroed() };
+}
 
 /// Create a new zeroed T.
 ///
@@ -1444,7 +1444,7 @@ macro_rules! impl_zeroable {
     {<T>} Opaque<T>,
 
     // SAFETY: `T: Zeroable` and `UnsafeCell` is `repr(transparent)`.
-    {<T: ?Sized + Zeroable>} UnsafeCell<T>,
+    {<T: Zeroable>} UnsafeCell<T>,
 
     // SAFETY: All zeros is equivalent to `None` (option layout optimization guarantee).
     Option<NonZeroU8>, Option<NonZeroU16>, Option<NonZeroU32>, Option<NonZeroU64>,
-- 
2.47.0

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

[Paolo Bonzini]

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@redhat.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 {}
+
 impl<T: Operations> TagSet<T> {
     /// Try to create a new tag set
     pub fn new(
@@ -39,9 +44,6 @@ pub fn new(
         num_tags: u32,
         num_maps: u32,
     ) -> impl PinInit<Self, error::Error> {
-        // SAFETY: `blk_mq_tag_set` only contains integers and pointers, which
-        // all are allowed to be 0.
-        let tag_set: bindings::blk_mq_tag_set = unsafe { core::mem::zeroed() };
         let tag_set = core::mem::size_of::<RequestDataWrapper>()
             .try_into()
             .map(|cmd_size| {
@@ -55,7 +57,7 @@ pub fn new(
                     flags: bindings::BLK_MQ_F_SHOULD_MERGE,
                     driver_data: core::ptr::null_mut::<core::ffi::c_void>(),
                     nr_maps: num_maps,
-                    ..tag_set
+                    ..Zeroable::ZERO
                 }
             });
 
-- 
2.47.0

Re: [PATCH 1/2] rust: Zeroable: allow struct update syntax outside init macros

[Alice Ryhl]

On Thu, Nov 28, 2024 at 3:13 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The Zeroable trait is a marker trait, even though the various init macros
> use a "fake" struct update syntax.  Sometimes, such a struct update
> syntax can be useful even outside the init macros.  Add an associated
> const that returns an all-zero instance of a Zeroable type.
>
> The exact syntax used by the init macros cannot be reproduced without
> forgoing the ability to use Zeroable::ZERO in const context.  However,
> it might not be a good idea to add a fn zeroed() inside the
> Zeroable trait, to avoid confusion with the init::zeroed() function
> and because Zeroable::ZERO is unrelated to the Init and PinInit
> traits.  In other words, let's treat this difference as a
> feature rather than a bug.
>
> The definition of the ZERO constant requires adding a Sized boundary, but
> this is not a problem either because neither slices nor trait objects
> are zeroable.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Slices are zeroable. I know they don't implement the trait, but they
could implement it, and this could be used to implement e.g.:

pub fn write_zero<T: Zeroed + ?Sized>(value: &mut T) {
    memset(0, ...);
}

Alice

Re: [PATCH 1/2] rust: Zeroable: allow struct update syntax outside init macros

[Paolo Bonzini]

On 11/28/24 15:40, Alice Ryhl wrote:
>> The definition of the ZERO constant requires adding a Sized boundary, but
>> this is not a problem either because neither slices nor trait objects
>> are zeroable.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Slices are zeroable. I know they don't implement the trait,

Right, I should have used the uppercase "Zeroable" for clarity.

> but they could implement it, and this could be used to implement e.g.:
> 
> pub fn write_zero<T: Zeroed + ?Sized>(value: &mut T) {
>      memset(0, ...);
> }

Yeah, that would be I think

     pub fn write_zero<T: Zeroable + ?Sized>(value: &mut T) {
         unsafe {
            ptr::write_bytes((value as *mut T).cast::<u8>(), 0,
                             std::mem::size_of_val(value))
         }
     }

?  And it works for both sized values and slices.  If Zeroable is 
limited to sized types, I guess you could still do:

     pub fn write_zero_slice<T: Zeroable>(value: &mut [T]) {
         ptr::write_bytes(value.as_mut_ptr(), 0, value.len())
     }

So the question is whether the ZERO constant is worthwhile enough, to 
justify the limitation of the Sized bound (e.g. having separate 
write_zero and write_zero_slice in the future).

Thanks,

Paolo

Re: [PATCH 1/2] rust: Zeroable: allow struct update syntax outside init macros

[Alice Ryhl]

On Thu, Nov 28, 2024 at 5:43 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 11/28/24 15:40, Alice Ryhl wrote:
> >> The definition of the ZERO constant requires adding a Sized boundary, but
> >> this is not a problem either because neither slices nor trait objects
> >> are zeroable.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >
> > Slices are zeroable. I know they don't implement the trait,
>
> Right, I should have used the uppercase "Zeroable" for clarity.
>
> > but they could implement it, and this could be used to implement e.g.:
> >
> > pub fn write_zero<T: Zeroed + ?Sized>(value: &mut T) {
> >      memset(0, ...);
> > }
>
> Yeah, that would be I think
>
>      pub fn write_zero<T: Zeroable + ?Sized>(value: &mut T) {
>          unsafe {
>             ptr::write_bytes((value as *mut T).cast::<u8>(), 0,
>                              std::mem::size_of_val(value))
>          }
>      }
>
> ?  And it works for both sized values and slices.  If Zeroable is
> limited to sized types, I guess you could still do:
>
>      pub fn write_zero_slice<T: Zeroable>(value: &mut [T]) {
>          ptr::write_bytes(value.as_mut_ptr(), 0, value.len())
>      }
>
> So the question is whether the ZERO constant is worthwhile enough, to
> justify the limitation of the Sized bound (e.g. having separate
> write_zero and write_zero_slice in the future).

Why not both?

If you change the constant to a const fn, then you don't have to rule
out either use-case.

Alice

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

[Alice Ryhl]

On Thu, Nov 28, 2024 at 3:13 PM Paolo Bonzini <pbonzini@redhat.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@redhat.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

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

[Paolo Bonzini]

On 11/29/24 10:41, Alice Ryhl wrote:
> On Thu, Nov 28, 2024 at 3:13 PM Paolo Bonzini <pbonzini@redhat.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@redhat.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.

Ok, I will look into using the bindgen doc comments then.  It looks like 
the idea is not dead on arrival at least!  Thanks for the review.

Paolo