public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] rust: pin-init: fix incorrect accessor reference lifetime
@ 2026-04-23 14:51 Gary Guo
  2026-04-23 14:51 ` [PATCH v2 1/2] rust: pin-init: internal: move alignment check to `make_field_check` Gary Guo
  2026-04-23 14:51 ` [PATCH v2 2/2] rust: pin-init: fix incorrect accessor reference lifetime Gary Guo
  0 siblings, 2 replies; 6+ messages in thread
From: Gary Guo @ 2026-04-23 14:51 UTC (permalink / raw)
  To: Benno Lossin, Gary Guo, Miguel Ojeda, Boqun Feng,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Danilo Krummrich
  Cc: rust-for-linux, linux-kernel, stable

When a field has been initialized, `init!`/`pin_init!` create a reference
or pinned reference to the field so it can be accessed later during the
initialization of other fields. However, the reference it created is
incorrectly `&'static` rather than just the scope of the initializer.

This means that you can do

    init!(Foo {
        a: 1,
        _: {
            let b: &'static u32 = a;
        }
    })

which is unsound.

This series fix the issue. Details can be found in the second patch.

Changes since v1:
- Moved the field alignment check as the current dual-purpose reference taking
  for guard and for unaligned fields cause trouble when refactoring.
- Use a method instead of `DerefMut` operator as we don't need the `Deref`.
- Reworked `DropGuard` to use a reference to capture the safety invariants
  (Sashiko)
- Generally improved the safety comments.
- Link to v1: https://lore.kernel.org/rust-for-linux/20260420172302.1843752-1-gary@kernel.org

---
Gary Guo (2):
      rust: pin-init: internal: move alignment check to `make_field_check`
      rust: pin-init: fix incorrect accessor reference lifetime

 rust/pin-init/internal/src/init.rs | 182 +++++++++++++++++--------------------
 rust/pin-init/src/__internal.rs    |  31 ++++---
 2 files changed, 99 insertions(+), 114 deletions(-)
---
base-commit: 97e797263a5e963da3d1e66e743fd518567dfe37
change-id: 20260423-pin-init-fix-cf469cd6f782

Best regards,
--  
Gary Guo <gary@garyguo.net>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/2] rust: pin-init: internal: move alignment check to `make_field_check`
  2026-04-23 14:51 [PATCH v2 0/2] rust: pin-init: fix incorrect accessor reference lifetime Gary Guo
@ 2026-04-23 14:51 ` Gary Guo
  2026-04-27  7:43   ` Alice Ryhl
  2026-04-23 14:51 ` [PATCH v2 2/2] rust: pin-init: fix incorrect accessor reference lifetime Gary Guo
  1 sibling, 1 reply; 6+ messages in thread
From: Gary Guo @ 2026-04-23 14:51 UTC (permalink / raw)
  To: Benno Lossin, Gary Guo, Miguel Ojeda, Boqun Feng,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Danilo Krummrich
  Cc: rust-for-linux, linux-kernel, stable

Instead of having the reference creation serving dual-purpose as both for
let bindings and alignment check, detangle them so that the alignment check
is done explicitly in `make_field_check`. This is more robust again
refactors that may change the way let bindings are created.

Cc: stable@vger.kernel.org
Signed-off-by: Gary Guo <gary@garyguo.net>
---
 rust/pin-init/internal/src/init.rs | 78 ++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 41 deletions(-)

diff --git a/rust/pin-init/internal/src/init.rs b/rust/pin-init/internal/src/init.rs
index daa3f1c6466e..0a6600e8156c 100644
--- a/rust/pin-init/internal/src/init.rs
+++ b/rust/pin-init/internal/src/init.rs
@@ -249,10 +249,6 @@ fn init_fields(
                 });
                 // Again span for better diagnostics
                 let write = quote_spanned!(ident.span()=> ::core::ptr::write);
-                // NOTE: the field accessor ensures that the initialized field is properly aligned.
-                // Unaligned fields will cause the compiler to emit E0793. We do not support
-                // unaligned fields since `Init::__init` requires an aligned pointer; the call to
-                // `ptr::write` below has the same requirement.
                 let accessor = if pinned {
                     let project_ident = format_ident!("__project_{ident}");
                     quote! {
@@ -367,49 +363,49 @@ fn init_fields(
     }
 }
 
-/// Generate the check for ensuring that every field has been initialized.
+/// Generate the check for ensuring that every field has been initialized and aligned.
 fn make_field_check(
     fields: &Punctuated<InitializerField, Token![,]>,
     init_kind: InitKind,
     path: &Path,
 ) -> TokenStream {
-    let field_attrs = fields
+    let field_attrs: Vec<_> = fields
         .iter()
-        .filter_map(|f| f.kind.ident().map(|_| &f.attrs));
-    let field_name = fields.iter().filter_map(|f| f.kind.ident());
-    match init_kind {
-        InitKind::Normal => quote! {
-            // We use unreachable code to ensure that all fields have been mentioned exactly once,
-            // this struct initializer will still be type-checked and complain with a very natural
-            // error message if a field is forgotten/mentioned more than once.
-            #[allow(unreachable_code, clippy::diverging_sub_expression)]
-            // SAFETY: this code is never executed.
-            let _ = || unsafe {
-                ::core::ptr::write(slot, #path {
-                    #(
-                        #(#field_attrs)*
-                        #field_name: ::core::panic!(),
-                    )*
-                })
-            };
-        },
-        InitKind::Zeroing => quote! {
-            // We use unreachable code to ensure that all fields have been mentioned at most once.
-            // Since the user specified `..Zeroable::zeroed()` at the end, all missing fields will
-            // be zeroed. This struct initializer will still be type-checked and complain with a
-            // very natural error message if a field is mentioned more than once, or doesn't exist.
-            #[allow(unreachable_code, clippy::diverging_sub_expression, unused_assignments)]
-            // SAFETY: this code is never executed.
-            let _ = || unsafe {
-                ::core::ptr::write(slot, #path {
-                    #(
-                        #(#field_attrs)*
-                        #field_name: ::core::panic!(),
-                    )*
-                    ..::core::mem::zeroed()
-                })
-            };
-        },
+        .filter_map(|f| f.kind.ident().map(|_| &f.attrs))
+        .collect();
+    let field_name: Vec<_> = fields.iter().filter_map(|f| f.kind.ident()).collect();
+    let zeroing_trailer = match init_kind {
+        InitKind::Normal => None,
+        InitKind::Zeroing => Some(quote! {
+            ..::core::mem::zeroed()
+        }),
+    };
+    quote! {
+        #[allow(unreachable_code, clippy::diverging_sub_expression)]
+        // We use unreachable code to perform field checks. They're still checked by the compiler.
+        // SAFETY: this code is never executed.
+        let _ = || unsafe {
+            // Create references to ensure that the initialized field is properly aligned.
+            // Unaligned fields will cause the compiler to emit E0793. We do not support
+            // unaligned fields since `Init::__init` requires an aligned pointer; the call to
+            // `ptr::write` for value-initialization case has the same requirement.
+            #(
+                #(#field_attrs)*
+                let _ = &(*slot).#field_name;
+            )*
+
+            // If the zeroing trailer is not present, this checks that all fields have been
+            // mentioned exactly once. If the zeroing trailer is present, all missing fields will be
+            // zeroed, so this checks that all fields have been mentioned at most once. The use of
+            // struct initializer will still generate very natural error messages for any misuse.
+            ::core::ptr::write(slot, #path {
+                #(
+                    #(#field_attrs)*
+                    #field_name: ::core::panic!(),
+                )*
+                #zeroing_trailer
+            })
+        };
     }
 }
 

-- 
2.51.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/2] rust: pin-init: fix incorrect accessor reference lifetime
  2026-04-23 14:51 [PATCH v2 0/2] rust: pin-init: fix incorrect accessor reference lifetime Gary Guo
  2026-04-23 14:51 ` [PATCH v2 1/2] rust: pin-init: internal: move alignment check to `make_field_check` Gary Guo
@ 2026-04-23 14:51 ` Gary Guo
  2026-04-24 19:10   ` Gary Guo
  1 sibling, 1 reply; 6+ messages in thread
From: Gary Guo @ 2026-04-23 14:51 UTC (permalink / raw)
  To: Benno Lossin, Gary Guo, Miguel Ojeda, Boqun Feng,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Danilo Krummrich
  Cc: rust-for-linux, linux-kernel, stable

When a field has been initialized, `init!`/`pin_init!` create a reference
or pinned reference to the field so it can be accessed later during the
initialization of other fields. However, the reference it created is
incorrectly `&'static` rather than just the scope of the initializer.

This means that you can do

    init!(Foo {
        a: 1,
        _: {
            let b: &'static u32 = a;
        }
    })

which is unsound.

This is caused by `&mut (*#slot).#ident`, which actually allows arbitrary
lifetime, so this is effectively `'static`. Somewhat ironically, the safety
justification of creating the accessor is.. "SAFETY: TODO".

Fix it by adding `let_binding` method on `DropGuard` to shorten lifetime.
This results exactly what we want for these accessors.

Fixes: 42415d163e5d ("rust: pin-init: add references to previously initialized fields")
Cc: stable@vger.kernel.org
Signed-off-by: Gary Guo <gary@garyguo.net>
---
 rust/pin-init/internal/src/init.rs | 104 ++++++++++++++++---------------------
 rust/pin-init/src/__internal.rs    |  31 ++++++-----
 2 files changed, 62 insertions(+), 73 deletions(-)

diff --git a/rust/pin-init/internal/src/init.rs b/rust/pin-init/internal/src/init.rs
index 0a6600e8156c..ad383023c21a 100644
--- a/rust/pin-init/internal/src/init.rs
+++ b/rust/pin-init/internal/src/init.rs
@@ -249,18 +249,6 @@ fn init_fields(
                 });
                 // Again span for better diagnostics
                 let write = quote_spanned!(ident.span()=> ::core::ptr::write);
-                let accessor = if pinned {
-                    let project_ident = format_ident!("__project_{ident}");
-                    quote! {
-                        // SAFETY: TODO
-                        unsafe { #data.#project_ident(&mut (*#slot).#ident) }
-                    }
-                } else {
-                    quote! {
-                        // SAFETY: TODO
-                        unsafe { &mut (*#slot).#ident }
-                    }
-                };
                 quote! {
                     #(#attrs)*
                     {
@@ -268,51 +256,31 @@ fn init_fields(
                         // SAFETY: TODO
                         unsafe { #write(&raw mut (*#slot).#ident, #value_ident) };
                     }
-                    #(#cfgs)*
-                    #[allow(unused_variables)]
-                    let #ident = #accessor;
                 }
             }
             InitializerKind::Init { ident, value, .. } => {
                 // Again span for better diagnostics
                 let init = format_ident!("init", span = value.span());
-                // NOTE: the field accessor ensures that the initialized field is properly aligned.
-                // Unaligned fields will cause the compiler to emit E0793. We do not support
-                // unaligned fields since `Init::__init` requires an aligned pointer; the call to
-                // `ptr::write` below has the same requirement.
-                let (value_init, accessor) = if pinned {
-                    let project_ident = format_ident!("__project_{ident}");
-                    (
-                        quote! {
-                            // SAFETY:
-                            // - `slot` is valid, because we are inside of an initializer closure, we
-                            //   return when an error/panic occurs.
-                            // - We also use `#data` to require the correct trait (`Init` or `PinInit`)
-                            //   for `#ident`.
-                            unsafe { #data.#ident(&raw mut (*#slot).#ident, #init)? };
-                        },
-                        quote! {
-                            // SAFETY: TODO
-                            unsafe { #data.#project_ident(&mut (*#slot).#ident) }
-                        },
-                    )
+                let value_init = if pinned {
+                    quote! {
+                        // SAFETY:
+                        // - `slot` is valid, because we are inside of an initializer closure, we
+                        //   return when an error/panic occurs.
+                        // - We also use `#data` to require the correct trait (`Init` or `PinInit`)
+                        //   for `#ident`.
+                        unsafe { #data.#ident(&raw mut (*#slot).#ident, #init)? };
+                    }
                 } else {
-                    (
-                        quote! {
-                            // SAFETY: `slot` is valid, because we are inside of an initializer
-                            // closure, we return when an error/panic occurs.
-                            unsafe {
-                                ::pin_init::Init::__init(
-                                    #init,
-                                    &raw mut (*#slot).#ident,
-                                )?
-                            };
-                        },
-                        quote! {
-                            // SAFETY: TODO
-                            unsafe { &mut (*#slot).#ident }
-                        },
-                    )
+                    quote! {
+                        // SAFETY: `slot` is valid, because we are inside of an initializer
+                        // closure, we return when an error/panic occurs.
+                        unsafe {
+                            ::pin_init::Init::__init(
+                                #init,
+                                &raw mut (*#slot).#ident,
+                            )?
+                        };
+                    }
                 };
                 quote! {
                     #(#attrs)*
@@ -320,9 +288,6 @@ fn init_fields(
                         let #init = #value;
                         #value_init
                     }
-                    #(#cfgs)*
-                    #[allow(unused_variables)]
-                    let #ident = #accessor;
                 }
             }
             InitializerKind::Code { block: value, .. } => quote! {
@@ -335,18 +300,37 @@ fn init_fields(
         if let Some(ident) = kind.ident() {
             // `mixed_site` ensures that the guard is not accessible to the user-controlled code.
             let guard = format_ident!("__{ident}_guard", span = Span::mixed_site());
+
+            // NOTE: The reference is derived from the guard so that it only lives as long as the
+            // guard does and cannot escape the scope. If it's created via `&mut (*#slot).#ident`
+            // like the unaligned field guard, it will become effectively `'static`.
+            let accessor = if pinned {
+                let project_ident = format_ident!("__project_{ident}");
+                quote! {
+                    // SAFETY: the initialization is pinned.
+                    unsafe { #data.#project_ident(#guard.let_binding()) }
+                }
+            } else {
+                quote! {
+                    #guard.let_binding()
+                }
+            };
+
             res.extend(quote! {
                 #(#cfgs)*
-                // Create the drop guard:
+                // Create the drop guard.
                 //
-                // We rely on macro hygiene to make it impossible for users to access this local
-                // variable.
-                // SAFETY: We forget the guard later when initialization has succeeded.
-                let #guard = unsafe {
+                // SAFETY: We forget the guard later when initialization has succeeded. If we didn't
+                // forget it, they would not be further accessed again.
+                let mut #guard = unsafe {
                     ::pin_init::__internal::DropGuard::new(
-                        &raw mut (*slot).#ident
+                        &mut (*slot).#ident
                     )
                 };
+
+                #(#cfgs)*
+                #[allow(unused_variables)]
+                let #ident = #accessor;
             });
             guards.push(guard);
             guard_attrs.push(cfgs);
diff --git a/rust/pin-init/src/__internal.rs b/rust/pin-init/src/__internal.rs
index 90adbdc1893b..c3fd7589fd82 100644
--- a/rust/pin-init/src/__internal.rs
+++ b/rust/pin-init/src/__internal.rs
@@ -238,32 +238,37 @@ struct Foo {
 /// When a value of this type is dropped, it drops a `T`.
 ///
 /// Can be forgotten to prevent the drop.
-pub struct DropGuard<T: ?Sized> {
-    ptr: *mut T,
+///
+/// # Invariants
+///
+/// `ptr` will not be accessed or dropped after `DropGuard` is dropped.
+pub struct DropGuard<'a, T: ?Sized> {
+    ptr: &'a mut T,
 }
 
-impl<T: ?Sized> DropGuard<T> {
+impl<'a, T: ?Sized> DropGuard<'a, T> {
     /// Creates a new [`DropGuard<T>`]. It will [`ptr::drop_in_place`] `ptr` when it gets dropped.
     ///
     /// # Safety
     ///
-    /// `ptr` must be a valid pointer.
-    ///
-    /// It is the callers responsibility that `self` will only get dropped if the pointee of `ptr`:
-    /// - has not been dropped,
-    /// - is not accessible by any other means,
-    /// - will not be dropped by any other means.
+    /// `ptr` must not be accessed or dropped after `DropGuard` is dropped.
     #[inline]
-    pub unsafe fn new(ptr: *mut T) -> Self {
+    pub unsafe fn new(ptr: &'a mut T) -> Self {
+        // INVARIANT: By safety requirement.
         Self { ptr }
     }
+
+    /// Create a let binding for accessor use.
+    #[inline]
+    pub fn let_binding(&mut self) -> &mut T {
+        self.ptr
+    }
 }
 
-impl<T: ?Sized> Drop for DropGuard<T> {
+impl<T: ?Sized> Drop for DropGuard<'_, T> {
     #[inline]
     fn drop(&mut self) {
-        // SAFETY: A `DropGuard` can only be constructed using the unsafe `new` function
-        // ensuring that this operation is safe.
+        // SAFETY: `self.ptr` is not going to be accessed or dropped later.
         unsafe { ptr::drop_in_place(self.ptr) }
     }
 }

-- 
2.51.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/2] rust: pin-init: fix incorrect accessor reference lifetime
  2026-04-23 14:51 ` [PATCH v2 2/2] rust: pin-init: fix incorrect accessor reference lifetime Gary Guo
@ 2026-04-24 19:10   ` Gary Guo
  2026-04-27  7:43     ` Alice Ryhl
  0 siblings, 1 reply; 6+ messages in thread
From: Gary Guo @ 2026-04-24 19:10 UTC (permalink / raw)
  To: Gary Guo, Benno Lossin, Miguel Ojeda, Boqun Feng,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Danilo Krummrich
  Cc: rust-for-linux, linux-kernel, stable

On Thu Apr 23, 2026 at 3:51 PM BST, Gary Guo wrote:
> When a field has been initialized, `init!`/`pin_init!` create a reference
> or pinned reference to the field so it can be accessed later during the
> initialization of other fields. However, the reference it created is
> incorrectly `&'static` rather than just the scope of the initializer.
>
> This means that you can do
>
>     init!(Foo {
>         a: 1,
>         _: {
>             let b: &'static u32 = a;
>         }
>     })
>
> which is unsound.
>
> This is caused by `&mut (*#slot).#ident`, which actually allows arbitrary
> lifetime, so this is effectively `'static`. Somewhat ironically, the safety
> justification of creating the accessor is.. "SAFETY: TODO".
>
> Fix it by adding `let_binding` method on `DropGuard` to shorten lifetime.
> This results exactly what we want for these accessors.
>
> Fixes: 42415d163e5d ("rust: pin-init: add references to previously initialized fields")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
>  rust/pin-init/internal/src/init.rs | 104 ++++++++++++++++---------------------
>  rust/pin-init/src/__internal.rs    |  31 ++++++-----
>  2 files changed, 62 insertions(+), 73 deletions(-)

> diff --git a/rust/pin-init/src/__internal.rs b/rust/pin-init/src/__internal.rs
> index 90adbdc1893b..c3fd7589fd82 100644
> --- a/rust/pin-init/src/__internal.rs
> +++ b/rust/pin-init/src/__internal.rs
> @@ -238,32 +238,37 @@ struct Foo {
>  /// When a value of this type is dropped, it drops a `T`.
>  ///
>  /// Can be forgotten to prevent the drop.
> -pub struct DropGuard<T: ?Sized> {
> -    ptr: *mut T,
> +///
> +/// # Invariants
> +///
> +/// `ptr` will not be accessed or dropped after `DropGuard` is dropped.
> +pub struct DropGuard<'a, T: ?Sized> {
> +    ptr: &'a mut T,
>  }
>  
> -impl<T: ?Sized> DropGuard<T> {
> +impl<'a, T: ?Sized> DropGuard<'a, T> {
>      /// Creates a new [`DropGuard<T>`]. It will [`ptr::drop_in_place`] `ptr` when it gets dropped.
>      ///
>      /// # Safety
>      ///
> -    /// `ptr` must be a valid pointer.
> -    ///
> -    /// It is the callers responsibility that `self` will only get dropped if the pointee of `ptr`:
> -    /// - has not been dropped,
> -    /// - is not accessible by any other means,
> -    /// - will not be dropped by any other means.
> +    /// `ptr` must not be accessed or dropped after `DropGuard` is dropped.
>      #[inline]
> -    pub unsafe fn new(ptr: *mut T) -> Self {
> +    pub unsafe fn new(ptr: &'a mut T) -> Self {
> +        // INVARIANT: By safety requirement.
>          Self { ptr }
>      }
> +
> +    /// Create a let binding for accessor use.
> +    #[inline]
> +    pub fn let_binding(&mut self) -> &mut T {
> +        self.ptr
> +    }
>  }
>  
> -impl<T: ?Sized> Drop for DropGuard<T> {
> +impl<T: ?Sized> Drop for DropGuard<'_, T> {
>      #[inline]
>      fn drop(&mut self) {
> -        // SAFETY: A `DropGuard` can only be constructed using the unsafe `new` function
> -        // ensuring that this operation is safe.
> +        // SAFETY: `self.ptr` is not going to be accessed or dropped later.
>          unsafe { ptr::drop_in_place(self.ptr) }
>      }
>  }

Sashiko mentions that:

> When ptr::drop_in_place(self.ptr) is called here, the value is dropped,
> but the DropGuard struct still holds the &'a mut T field until the
> drop method completely returns.
> 
> Would it be better to revert DropGuard to store a raw pointer and use
> unsafe { &mut *self.ptr } in let_binding instead?
> 
> The lifetime-shortening effect is fully achieved by the let_binding
> signature taking &mut self and returning &mut T, which ties the returned
> reference to the local borrow of the guard variable. This avoids the
> potential validity issues while fully preserving the bug fix.

which has a point but not totally correct as the code is not violating the
validity invariants of references, just the safety invariants. And since no code
executed can observe the violation, the code is not undefined. The code passes
all Miri checks which pin-init CI runs with both aliasing models.

I only used reference here because it's more convenient to do so (less safety
comments to write), but if the effect is that it's harder to justify the
correctness (and apparently Sashiko got confused here), then it's not worth
doing and I should just spell out all safety comments repetitively.

I'll send a new version with the approach reverted to pointers. PATCH 1/2 will
be kept as is.

Best,
Gary

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] rust: pin-init: internal: move alignment check to `make_field_check`
  2026-04-23 14:51 ` [PATCH v2 1/2] rust: pin-init: internal: move alignment check to `make_field_check` Gary Guo
@ 2026-04-27  7:43   ` Alice Ryhl
  0 siblings, 0 replies; 6+ messages in thread
From: Alice Ryhl @ 2026-04-27  7:43 UTC (permalink / raw)
  To: Gary Guo
  Cc: Benno Lossin, Miguel Ojeda, Boqun Feng, Björn Roy Baron,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel, stable

On Thu, Apr 23, 2026 at 03:51:49PM +0100, Gary Guo wrote:
> Instead of having the reference creation serving dual-purpose as both for
> let bindings and alignment check, detangle them so that the alignment check
> is done explicitly in `make_field_check`. This is more robust again
> refactors that may change the way let bindings are created.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Gary Guo <gary@garyguo.net>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/2] rust: pin-init: fix incorrect accessor reference lifetime
  2026-04-24 19:10   ` Gary Guo
@ 2026-04-27  7:43     ` Alice Ryhl
  0 siblings, 0 replies; 6+ messages in thread
From: Alice Ryhl @ 2026-04-27  7:43 UTC (permalink / raw)
  To: Gary Guo
  Cc: Benno Lossin, Miguel Ojeda, Boqun Feng, Björn Roy Baron,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich, rust-for-linux,
	linux-kernel, stable

On Fri, Apr 24, 2026 at 08:10:06PM +0100, Gary Guo wrote:
> On Thu Apr 23, 2026 at 3:51 PM BST, Gary Guo wrote:
> > When a field has been initialized, `init!`/`pin_init!` create a reference
> > or pinned reference to the field so it can be accessed later during the
> > initialization of other fields. However, the reference it created is
> > incorrectly `&'static` rather than just the scope of the initializer.
> >
> > This means that you can do
> >
> >     init!(Foo {
> >         a: 1,
> >         _: {
> >             let b: &'static u32 = a;
> >         }
> >     })
> >
> > which is unsound.
> >
> > This is caused by `&mut (*#slot).#ident`, which actually allows arbitrary
> > lifetime, so this is effectively `'static`. Somewhat ironically, the safety
> > justification of creating the accessor is.. "SAFETY: TODO".
> >
> > Fix it by adding `let_binding` method on `DropGuard` to shorten lifetime.
> > This results exactly what we want for these accessors.
> >
> > Fixes: 42415d163e5d ("rust: pin-init: add references to previously initialized fields")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Gary Guo <gary@garyguo.net>
> > ---
> >  rust/pin-init/internal/src/init.rs | 104 ++++++++++++++++---------------------
> >  rust/pin-init/src/__internal.rs    |  31 ++++++-----
> >  2 files changed, 62 insertions(+), 73 deletions(-)
> 
> > diff --git a/rust/pin-init/src/__internal.rs b/rust/pin-init/src/__internal.rs
> > index 90adbdc1893b..c3fd7589fd82 100644
> > --- a/rust/pin-init/src/__internal.rs
> > +++ b/rust/pin-init/src/__internal.rs
> > @@ -238,32 +238,37 @@ struct Foo {
> >  /// When a value of this type is dropped, it drops a `T`.
> >  ///
> >  /// Can be forgotten to prevent the drop.
> > -pub struct DropGuard<T: ?Sized> {
> > -    ptr: *mut T,
> > +///
> > +/// # Invariants
> > +///
> > +/// `ptr` will not be accessed or dropped after `DropGuard` is dropped.
> > +pub struct DropGuard<'a, T: ?Sized> {
> > +    ptr: &'a mut T,
> >  }
> >  
> > -impl<T: ?Sized> DropGuard<T> {
> > +impl<'a, T: ?Sized> DropGuard<'a, T> {
> >      /// Creates a new [`DropGuard<T>`]. It will [`ptr::drop_in_place`] `ptr` when it gets dropped.
> >      ///
> >      /// # Safety
> >      ///
> > -    /// `ptr` must be a valid pointer.
> > -    ///
> > -    /// It is the callers responsibility that `self` will only get dropped if the pointee of `ptr`:
> > -    /// - has not been dropped,
> > -    /// - is not accessible by any other means,
> > -    /// - will not be dropped by any other means.
> > +    /// `ptr` must not be accessed or dropped after `DropGuard` is dropped.
> >      #[inline]
> > -    pub unsafe fn new(ptr: *mut T) -> Self {
> > +    pub unsafe fn new(ptr: &'a mut T) -> Self {
> > +        // INVARIANT: By safety requirement.
> >          Self { ptr }
> >      }
> > +
> > +    /// Create a let binding for accessor use.
> > +    #[inline]
> > +    pub fn let_binding(&mut self) -> &mut T {
> > +        self.ptr
> > +    }
> >  }
> >  
> > -impl<T: ?Sized> Drop for DropGuard<T> {
> > +impl<T: ?Sized> Drop for DropGuard<'_, T> {
> >      #[inline]
> >      fn drop(&mut self) {
> > -        // SAFETY: A `DropGuard` can only be constructed using the unsafe `new` function
> > -        // ensuring that this operation is safe.
> > +        // SAFETY: `self.ptr` is not going to be accessed or dropped later.
> >          unsafe { ptr::drop_in_place(self.ptr) }
> >      }
> >  }
> 
> Sashiko mentions that:
> 
> > When ptr::drop_in_place(self.ptr) is called here, the value is dropped,
> > but the DropGuard struct still holds the &'a mut T field until the
> > drop method completely returns.
> > 
> > Would it be better to revert DropGuard to store a raw pointer and use
> > unsafe { &mut *self.ptr } in let_binding instead?
> > 
> > The lifetime-shortening effect is fully achieved by the let_binding
> > signature taking &mut self and returning &mut T, which ties the returned
> > reference to the local borrow of the guard variable. This avoids the
> > potential validity issues while fully preserving the bug fix.
> 
> which has a point but not totally correct as the code is not violating the
> validity invariants of references, just the safety invariants. And since no code
> executed can observe the violation, the code is not undefined. The code passes
> all Miri checks which pin-init CI runs with both aliasing models.
> 
> I only used reference here because it's more convenient to do so (less safety
> comments to write), but if the effect is that it's harder to justify the
> correctness (and apparently Sashiko got confused here), then it's not worth
> doing and I should just spell out all safety comments repetitively.
> 
> I'll send a new version with the approach reverted to pointers. PATCH 1/2 will
> be kept as is.

Sounds reasonable to me.

Alice

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-04-27  7:44 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-23 14:51 [PATCH v2 0/2] rust: pin-init: fix incorrect accessor reference lifetime Gary Guo
2026-04-23 14:51 ` [PATCH v2 1/2] rust: pin-init: internal: move alignment check to `make_field_check` Gary Guo
2026-04-27  7:43   ` Alice Ryhl
2026-04-23 14:51 ` [PATCH v2 2/2] rust: pin-init: fix incorrect accessor reference lifetime Gary Guo
2026-04-24 19:10   ` Gary Guo
2026-04-27  7:43     ` Alice Ryhl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox