| 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
| ||
|
Message-ID: <f4bcd952-1ad9-42b7-6e0b-72a115dcbe8e@proton.me>
Date: Sat, 24 Jun 2023 21:14:47 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Björn Roy Baron <bjorn3_gh@...tonmail.com>
Cc: Miguel Ojeda <ojeda@...nel.org>,
Wedson Almeida Filho <wedsonaf@...il.com>,
Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Alice Ryhl <aliceryhl@...gle.com>,
Andreas Hindborg <nmi@...aspace.dk>,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
patches@...ts.linux.dev, Asahi Lina <lina@...hilina.net>
Subject: Re: [PATCH 5/7] rust: init: add `..Zeroable::zeroed()` syntax for zeroing all missing fields
On 6/24/23 17:11, Björn Roy Baron wrote:
> On Saturday, June 24th, 2023 at 11:25, Benno Lossin <benno.lossin@...ton.me> wrote:
>
>> Add the struct update syntax to the init macros, but only for
>> `..Zeroable::zeroed()`. Adding this at the end of the struct initializer
>> allows one to omit fields from the initializer, these fields will be
>> initialized with 0x00 set to every byte. Only types that implement the
>> `Zeroable` trait can utilize this.
>>
>> Suggested-by: Asahi Lina <lina@...hilina.net>
>> Signed-off-by: Benno Lossin <benno.lossin@...ton.me>
>> ---
>> rust/kernel/init.rs | 16 +++++-
>> rust/kernel/init/macros.rs | 114 ++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 128 insertions(+), 2 deletions(-)
>>
>> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
>> index ecf6a4bd0ce4..44bc3e77419a 100644
>> --- a/rust/kernel/init.rs
>> +++ b/rust/kernel/init.rs
>> @@ -508,14 +508,18 @@ macro_rules! stack_try_pin_init {
>> /// - Fields that you want to initialize in-place have to use `<-` instead of `:`.
>> /// - In front of the initializer you can write `&this in` to have access to a [`NonNull<Self>`]
>> /// pointer named `this` inside of the initializer.
>> +/// - Using struct update syntax one can place `..Zeroable::zeroed()` at the very end of the
>> +/// struct, this initializes every field with 0 and then runs all initializers specified in the
>> +/// body. This can only be done if [`Zeroable`] is implemented for the struct.
>> ///
>> /// For instance:
>> ///
>> /// ```rust
>> /// # use kernel::pin_init;
>> -/// # use macros::pin_data;
>> +/// # use macros::{pin_data, Zeroable};
>> /// # use core::{ptr::addr_of_mut, marker::PhantomPinned};
>> /// #[pin_data]
>> +/// #[derive(Zeroable)]
>> /// struct Buf {
>> /// // `ptr` points into `buf`.
>> /// ptr: *mut u8,
>> @@ -528,6 +532,10 @@ macro_rules! stack_try_pin_init {
>> /// ptr: unsafe { addr_of_mut!((*this.as_ptr()).buf).cast() },
>> /// pin: PhantomPinned,
>> /// });
>> +/// pin_init!(Buf {
>> +/// buf: [1; 64],
>> +/// ..Zeroable::zeroed(),
>> +/// });
>> /// ```
>> ///
>> /// [`try_pin_init!`]: kernel::try_pin_init
>> @@ -547,6 +555,7 @@ macro_rules! pin_init {
>> @data(PinData, use_data),
>> @has_data(HasPinData, __pin_data),
>> @construct_closure(pin_init_from_closure),
>> + @munch_fields($($fields)*),
>> )
>> };
>> }
>> @@ -603,6 +612,7 @@ macro_rules! try_pin_init {
>> @data(PinData, use_data),
>> @has_data(HasPinData, __pin_data),
>> @construct_closure(pin_init_from_closure),
>> + @munch_fields($($fields)*),
>> )
>> };
>> ($(&$this:ident in)? $t:ident $(::<$($generics:ty),* $(,)?>)? {
>> @@ -616,6 +626,7 @@ macro_rules! try_pin_init {
>> @data(PinData, use_data),
>> @has_data(HasPinData, __pin_data),
>> @construct_closure(pin_init_from_closure),
>> + @munch_fields($($fields)*),
>> )
>> };
>> }
>> @@ -650,6 +661,7 @@ macro_rules! init {
>> @data(InitData, /*no use_data*/),
>> @has_data(HasInitData, __init_data),
>> @construct_closure(init_from_closure),
>> + @munch_fields($($fields)*),
>> )
>> }
>> }
>> @@ -700,6 +712,7 @@ macro_rules! try_init {
>> @data(InitData, /*no use_data*/),
>> @has_data(HasInitData, __init_data),
>> @construct_closure(init_from_closure),
>> + @munch_fields($($fields)*),
>> )
>> };
>> ($(&$this:ident in)? $t:ident $(::<$($generics:ty),* $(,)?>)? {
>> @@ -713,6 +726,7 @@ macro_rules! try_init {
>> @data(InitData, /*no use_data*/),
>> @has_data(HasInitData, __init_data),
>> @construct_closure(init_from_closure),
>> + @munch_fields($($fields)*),
>> )
>> };
>> }
>> diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
>> index 1e0c4aca055a..5dcb2e513f26 100644
>> --- a/rust/kernel/init/macros.rs
>> +++ b/rust/kernel/init/macros.rs
>> @@ -989,6 +989,7 @@ impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
>> ///
>> /// This macro has multiple internal call configurations, these are always the very first ident:
>> /// - nothing: this is the base case and called by the `{try_}{pin_}init!` macros.
>> +/// - `with_update_parsed`: when the `..Zeroable::zeroed()` syntax has been handled.
>> /// - `init_slot`: recursively creates the code that initializes all fields in `slot`.
>> /// - `make_initializer`: recursively create the struct initializer that guarantees that every
>> /// field has been initialized exactly once.
>> @@ -1007,6 +1008,82 @@ macro_rules! __init_internal {
>> @has_data($has_data:ident, $get_data:ident),
>> // `pin_init_from_closure` or `init_from_closure`.
>> @construct_closure($construct_closure:ident),
>> + @munch_fields(),
>> + ) => {
>> + $crate::__init_internal!(with_update_parsed:
>> + @this($($this)?),
>> + @typ($t $(::<$($generics),*>)? ),
>> + @fields($($fields)*),
>> + @error($err),
>> + @data($data, $($use_data)?),
>> + @has_data($has_data, $get_data),
>> + @construct_closure($construct_closure),
>> + @zeroed(), // nothing means default behavior.
>> + )
>> + };
>> + (
>> + @this($($this:ident)?),
>> + @typ($t:ident $(::<$($generics:ty),*>)?),
>> + @fields($($fields:tt)*),
>> + @error($err:ty),
>> + // Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
>> + // case.
>> + @data($data:ident, $($use_data:ident)?),
>> + // `HasPinData` or `HasInitData`.
>> + @has_data($has_data:ident, $get_data:ident),
>> + // `pin_init_from_closure` or `init_from_closure`.
>> + @construct_closure($construct_closure:ident),
>> + @munch_fields(..Zeroable::zeroed()),
>> + ) => {
>> + $crate::__init_internal!(with_update_parsed:
>> + @this($($this)?),
>> + @typ($t $(::<$($generics),*>)? ),
>> + @fields($($fields)*),
>> + @error($err),
>> + @data($data, $($use_data)?),
>> + @has_data($has_data, $get_data),
>> + @construct_closure($construct_closure),
>> + @zeroed(()), // `()` means zero all fields not mentioned.
>> + )
>> + };
>> + (
>> + @this($($this:ident)?),
>> + @typ($t:ident $(::<$($generics:ty),*>)?),
>> + @fields($($fields:tt)*),
>> + @error($err:ty),
>> + // Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
>> + // case.
>> + @data($data:ident, $($use_data:ident)?),
>> + // `HasPinData` or `HasInitData`.
>> + @has_data($has_data:ident, $get_data:ident),
>> + // `pin_init_from_closure` or `init_from_closure`.
>> + @construct_closure($construct_closure:ident),
>> + @munch_fields($ignore:tt $($rest:tt)*),
>> + ) => {
>> + $crate::__init_internal!(
>> + @this($($this)?),
>> + @typ($t $(::<$($generics),*>)? ),
>> + @fields($($fields)*),
>> + @error($err),
>> + @data($data, $($use_data)?),
>> + @has_data($has_data, $get_data),
>> + @construct_closure($construct_closure),
>> + @munch_fields($($rest)*),
>> + )
>> + };
>> + (with_update_parsed:
>> + @this($($this:ident)?),
>> + @typ($t:ident $(::<$($generics:ty),*>)?),
>> + @fields($($fields:tt)*),
>> + @error($err:ty),
>> + // Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
>> + // case.
>> + @data($data:ident, $($use_data:ident)?),
>> + // `HasPinData` or `HasInitData`.
>> + @has_data($has_data:ident, $get_data:ident),
>> + // `pin_init_from_closure` or `init_from_closure`.
>> + @construct_closure($construct_closure:ident),
>> + @zeroed($($init_zeroed:expr)?),
>> ) => {{
>> // We do not want to allow arbitrary returns, so we declare this type as the `Ok` return
>> // type and shadow it later when we insert the arbitrary user code. That way there will be
>> @@ -1024,6 +1101,17 @@ macro_rules! __init_internal {
>> {
>> // Shadow the structure so it cannot be used to return early.
>> struct __InitOk;
>> + // If `$init_zeroed` is present we should zero the slot now and not emit an
>> + // error when fields are missing (since they will be zeroed). We also have to
>> + // check that the type actually implements `Zeroable`.
>> + $(
>> + fn is_zeroable<T: Zeroable>(ptr: *mut T) {}
>
> Maybe call this assert_zeroable?
Sure.
>
>> + // Ensure that the struct is indeed `Zeroable`.
>> + is_zeroable(slot);
>> + // SAFETY: The type implements `Zeroable` by the check above.
>> + unsafe { ::core::ptr::write_bytes(slot, 0, 1) };
>> + $init_zeroed // this will be `()` if set.
>
> How does this work? Shouldn't there be a ; after $init_zeroed to consume the () value?
It is the last expression of a block and since it is `()` it is ok
(adding a ; would also be ok, but it is not necessary).
>
>> + )?
>> // Create the `this` so it can be referenced by the user inside of the
>> // expressions creating the individual fields.
>> $(let $this = unsafe { ::core::ptr::NonNull::new_unchecked(slot) };)?
>> @@ -1064,7 +1152,7 @@ macro_rules! __init_internal {
>> @data($data:ident),
>> @slot($slot:ident),
>> @guards($($guards:ident,)*),
>> - @munch_fields($(,)?),
>> + @munch_fields($(..Zeroable::zeroed())? $(,)?),
>> ) => {
>> // Endpoint of munching, no fields are left. If execution reaches this point, all fields
>> // have been initialized. Therefore we can now dismiss the guards by forgetting them.
>> @@ -1157,6 +1245,30 @@ macro_rules! __init_internal {
>> @munch_fields($($rest)*),
>> );
>> };
>> + (make_initializer:
>> + @slot($slot:ident),
>> + @type_name($t:ident),
>> + @munch_fields(..Zeroable::zeroed() $(,)?),
>> + @acc($($acc:tt)*),
>> + ) => {
>> + // Endpoint, nothing more to munch, create the initializer. Since the users specified
>> + // `..Zeroable::zeroed()`, the slot will already have been zeroed and all field that have
>> + // not been overwritten are thus zero and initialized. We still check that all fields are
>> + // actually accessible by using the struct update syntax ourselves.
>> + // Since we are in the `if false` branch, this will never get executed. We abuse `slot` to
>> + // get the correct type inference here:
>> + unsafe {
>> + let mut zeroed = ::core::mem::zeroed();
>> + // We have to use type inference her to make zeroed have the correct type. This does
>
> *here
Will fix.
--
Cheers,
Benno
>
>> + // not get executed, so it has no effect.
>> + ::core::ptr::write($slot, zeroed);
>> + zeroed = ::core::mem::zeroed();
>> + ::core::ptr::write($slot, $t {
>> + $($acc)*
>> + ..zeroed
>> + });
>> + }
>> + };
>> (make_initializer:
>> @slot($slot:ident),
>> @type_name($t:ident),
>> --
>> 2.41.0
>
> Cheers,
> Björn
Powered by blists - more mailing lists