| 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: <20230802185233.138b81f1.gary@garyguo.net>
Date: Wed, 2 Aug 2023 18:52:33 +0100
From: Gary Guo <gary@...yguo.net>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: Miguel Ojeda <ojeda@...nel.org>,
Wedson Almeida Filho <wedsonaf@...il.com>,
Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Alice Ryhl <aliceryhl@...gle.com>,
Andreas Hindborg <nmi@...aspace.dk>,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
Asahi Lina <lina@...hilina.net>,
Martin Rodriguez Reboredo <yakoyoku@...il.com>
Subject: Re: [PATCH v3 04/13] rust: init: make guards in the init macros
hygienic
On Sat, 29 Jul 2023 09:09:40 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:
> Use hygienic identifiers for the guards instead of the field names. This
> makes the init macros feel more like normal struct initializers, since
> assigning identifiers with the name of a field does not create
> conflicts.
> Also change the internals of the guards, no need to make the `forget`
> function `unsafe`, since users cannot access the guards anyways. Now the
> guards are carried directly on the stack and have no extra `Cell<bool>`
> field that marks if they have been forgotten or not, instead they are
> just forgotten via `mem::forget`.
>
> Suggested-by: Asahi Lina <lina@...hilina.net>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@...il.com>
> Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>
> Signed-off-by: Benno Lossin <benno.lossin@...ton.me>
Reviewed-by: Gary Guo <gary@...yguo.net>
> ---
> v2 -> v3:
> - added Reviewed-by's from Martin and Alice.
>
> v1 -> v2:
> - use Gary's `paste!` macro to create the guard hygiene.
>
> rust/kernel/init.rs | 1 -
> rust/kernel/init/__internal.rs | 25 ++-----
> rust/kernel/init/macros.rs | 116 +++++++++++++++------------------
> 3 files changed, 56 insertions(+), 86 deletions(-)
>
> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> index d431d0b153a2..0120674b451e 100644
> --- a/rust/kernel/init.rs
> +++ b/rust/kernel/init.rs
> @@ -216,7 +216,6 @@
> use alloc::boxed::Box;
> use core::{
> alloc::AllocError,
> - cell::Cell,
> convert::Infallible,
> marker::PhantomData,
> mem::MaybeUninit,
> diff --git a/rust/kernel/init/__internal.rs b/rust/kernel/init/__internal.rs
> index 44751fb62b51..7abd1fb65e41 100644
> --- a/rust/kernel/init/__internal.rs
> +++ b/rust/kernel/init/__internal.rs
> @@ -174,7 +174,6 @@ pub fn init<E>(self: Pin<&mut Self>, init: impl PinInit<T, E>) -> Result<Pin<&mu
> /// Can be forgotten to prevent the drop.
> pub struct DropGuard<T: ?Sized> {
> ptr: *mut T,
> - do_drop: Cell<bool>,
> }
>
> impl<T: ?Sized> DropGuard<T> {
> @@ -190,32 +189,16 @@ impl<T: ?Sized> DropGuard<T> {
> /// - will not be dropped by any other means.
> #[inline]
> pub unsafe fn new(ptr: *mut T) -> Self {
> - Self {
> - ptr,
> - do_drop: Cell::new(true),
> - }
> - }
> -
> - /// Prevents this guard from dropping the supplied pointer.
> - ///
> - /// # Safety
> - ///
> - /// This function is unsafe in order to prevent safe code from forgetting this guard. It should
> - /// only be called by the macros in this module.
> - #[inline]
> - pub unsafe fn forget(&self) {
> - self.do_drop.set(false);
> + Self { ptr }
> }
> }
>
> impl<T: ?Sized> Drop for DropGuard<T> {
> #[inline]
> fn drop(&mut self) {
> - if self.do_drop.get() {
> - // SAFETY: A `DropGuard` can only be constructed using the unsafe `new` function
> - // ensuring that this operation is safe.
> - unsafe { ptr::drop_in_place(self.ptr) }
> - }
> + // SAFETY: A `DropGuard` can only be constructed using the unsafe `new` function
> + // ensuring that this operation is safe.
> + unsafe { ptr::drop_in_place(self.ptr) }
> }
> }
>
> diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
> index 78091756dec0..454f31b8c614 100644
> --- a/rust/kernel/init/macros.rs
> +++ b/rust/kernel/init/macros.rs
> @@ -994,7 +994,6 @@ impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
> /// - `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.
> -/// - `forget_guards`: recursively forget the drop guards for every field.
> #[doc(hidden)]
> #[macro_export]
> macro_rules! __init_internal {
> @@ -1034,6 +1033,7 @@ macro_rules! __init_internal {
> $crate::__init_internal!(init_slot($($use_data)?):
> @data(data),
> @slot(slot),
> + @guards(),
> @munch_fields($($fields)*,),
> );
> // We use unreachable code to ensure that all fields have been mentioned exactly
> @@ -1048,10 +1048,6 @@ macro_rules! __init_internal {
> @acc(),
> );
> }
> - // Forget all guards, since initialization was a success.
> - $crate::__init_internal!(forget_guards:
> - @munch_fields($($fields)*,),
> - );
> }
> Ok(__InitOk)
> }
> @@ -1065,13 +1061,17 @@ macro_rules! __init_internal {
> (init_slot($($use_data:ident)?):
> @data($data:ident),
> @slot($slot:ident),
> + @guards($($guards:ident,)*),
> @munch_fields($(,)?),
> ) => {
> - // Endpoint of munching, no fields are left.
> + // 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.
> + $(::core::mem::forget($guards);)*
> };
> (init_slot($use_data:ident): // use_data is present, so we use the `data` to init fields.
> @data($data:ident),
> @slot($slot:ident),
> + @guards($($guards:ident,)*),
> // In-place initialization syntax.
> @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
> ) => {
> @@ -1082,24 +1082,28 @@ macro_rules! __init_internal {
> // return when an error/panic occurs.
> // We also use the `data` to require the correct trait (`Init` or `PinInit`) for `$field`.
> unsafe { $data.$field(::core::ptr::addr_of_mut!((*$slot).$field), $field)? };
> - // Create the drop guard.
> - //
> - // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
> + // Create the drop guard:
> //
> - // SAFETY: We forget the guard later when initialization has succeeded.
> - let $field = &unsafe {
> - $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
> - };
> + // We rely on macro hygiene to make it impossible for users to access this local variable.
> + // We use `paste!` to create new hygiene for $field.
> + ::kernel::macros::paste! {
> + // SAFETY: We forget the guard later when initialization has succeeded.
> + let [<$field>] = unsafe {
> + $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
> + };
>
> - $crate::__init_internal!(init_slot($use_data):
> - @data($data),
> - @slot($slot),
> - @munch_fields($($rest)*),
> - );
> + $crate::__init_internal!(init_slot($use_data):
> + @data($data),
> + @slot($slot),
> + @guards([<$field>], $($guards,)*),
> + @munch_fields($($rest)*),
> + );
> + }
> };
> (init_slot(): // no use_data, so we use `Init::__init` directly.
> @data($data:ident),
> @slot($slot:ident),
> + @guards($($guards:ident,)*),
> // In-place initialization syntax.
> @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
> ) => {
> @@ -1109,24 +1113,28 @@ macro_rules! __init_internal {
> // SAFETY: `slot` is valid, because we are inside of an initializer closure, we
> // return when an error/panic occurs.
> unsafe { $crate::init::Init::__init($field, ::core::ptr::addr_of_mut!((*$slot).$field))? };
> - // Create the drop guard.
> - //
> - // We only give access to `&DropGuard`, so it cannot be forgotten via safe code.
> + // Create the drop guard:
> //
> - // SAFETY: We forget the guard later when initialization has succeeded.
> - let $field = &unsafe {
> - $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
> - };
> + // We rely on macro hygiene to make it impossible for users to access this local variable.
> + // We use `paste!` to create new hygiene for $field.
> + ::kernel::macros::paste! {
> + // SAFETY: We forget the guard later when initialization has succeeded.
> + let [<$field>] = unsafe {
> + $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
> + };
>
> - $crate::__init_internal!(init_slot():
> - @data($data),
> - @slot($slot),
> - @munch_fields($($rest)*),
> - );
> + $crate::__init_internal!(init_slot():
> + @data($data),
> + @slot($slot),
> + @guards([<$field>], $($guards,)*),
> + @munch_fields($($rest)*),
> + );
> + }
> };
> (init_slot($($use_data:ident)?):
> @data($data:ident),
> @slot($slot:ident),
> + @guards($($guards:ident,)*),
> // Init by-value.
> @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
> ) => {
> @@ -1137,18 +1145,21 @@ macro_rules! __init_internal {
> unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) };
> // Create the drop guard:
> //
> - // We only give access to `&DropGuard`, so it cannot be accidentally forgotten.
> - //
> - // SAFETY: We forget the guard later when initialization has succeeded.
> - let $field = &unsafe {
> - $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
> - };
> + // We rely on macro hygiene to make it impossible for users to access this local variable.
> + // We use `paste!` to create new hygiene for $field.
> + ::kernel::macros::paste! {
> + // SAFETY: We forget the guard later when initialization has succeeded.
> + let [<$field>] = unsafe {
> + $crate::init::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field))
> + };
>
> - $crate::__init_internal!(init_slot($($use_data)?):
> - @data($data),
> - @slot($slot),
> - @munch_fields($($rest)*),
> - );
> + $crate::__init_internal!(init_slot($($use_data)?):
> + @data($data),
> + @slot($slot),
> + @guards([<$field>], $($guards,)*),
> + @munch_fields($($rest)*),
> + );
> + }
> };
> (make_initializer:
> @slot($slot:ident),
> @@ -1191,29 +1202,6 @@ macro_rules! __init_internal {
> @acc($($acc)* $field: ::core::panic!(),),
> );
> };
> - (forget_guards:
> - @munch_fields($(,)?),
> - ) => {
> - // Munching finished.
> - };
> - (forget_guards:
> - @munch_fields($field:ident <- $val:expr, $($rest:tt)*),
> - ) => {
> - unsafe { $crate::init::__internal::DropGuard::forget($field) };
> -
> - $crate::__init_internal!(forget_guards:
> - @munch_fields($($rest)*),
> - );
> - };
> - (forget_guards:
> - @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
> - ) => {
> - unsafe { $crate::init::__internal::DropGuard::forget($field) };
> -
> - $crate::__init_internal!(forget_guards:
> - @munch_fields($($rest)*),
> - );
> - };
> }
>
> #[doc(hidden)]
Powered by blists - more mailing lists